{"action":"created","issue":{"url":"https://api.github.com/repos/nodejs/node/issues/18936","repository_url":"https://api.github.com/repos/nodejs/node","labels_url":"https://api.github.com/repos/nodejs/node/issues/18936/labels{/name}","comments_url":"https://api.github.com/repos/nodejs/node/issues/18936/comments","events_url":"https://api.github.com/repos/nodejs/node/issues/18936/events","html_url":"https://github.com/nodejs/node/pull/18936","id":299369904,"number":18936,"title":"src,http2: use native-layer pipe mechanism from FileHandle instead of synchronous I/O","user":{"login":"addaleax","id":899444,"avatar_url":"https://avatars2.githubusercontent.com/u/899444?v=4","gravatar_id":"","url":"https://api.github.com/users/addaleax","html_url":"https://github.com/addaleax","followers_url":"https://api.github.com/users/addaleax/followers","following_url":"https://api.github.com/users/addaleax/following{/other_user}","gists_url":"https://api.github.com/users/addaleax/gists{/gist_id}","starred_url":"https://api.github.com/users/addaleax/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/addaleax/subscriptions","organizations_url":"https://api.github.com/users/addaleax/orgs","repos_url":"https://api.github.com/users/addaleax/repos","events_url":"https://api.github.com/users/addaleax/events{/privacy}","received_events_url":"https://api.github.com/users/addaleax/received_events","type":"User","site_admin":false},"labels":[{"id":155267502,"url":"https://api.github.com/repos/nodejs/node/labels/C++","name":"C++","color":"e11d21","default":false},{"id":744850384,"url":"https://api.github.com/repos/nodejs/node/labels/backport-requested-v9.x","name":"backport-requested-v9.x","color":"0e8a16","default":false},{"id":647282877,"url":"https://api.github.com/repos/nodejs/node/labels/http2","name":"http2","color":"c2e0c6","default":false},{"id":281232761,"url":"https://api.github.com/repos/nodejs/node/labels/lib%20/%20src","name":"lib / src","color":"fad8c7","default":false}],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":29,"created_at":"2018-02-22T14:16:29Z","updated_at":"2018-03-18T14:01:29Z","closed_at":"2018-03-15T11:54:57Z","author_association":"OWNER","pull_request":{"url":"https://api.github.com/repos/nodejs/node/pulls/18936","html_url":"https://github.com/nodejs/node/pull/18936","diff_url":"https://github.com/nodejs/node/pull/18936.diff","patch_url":"https://github.com/nodejs/node/pull/18936.patch"},"body":"Hi everyone! :)\r\n\r\nThis PR resolves the issue of using synchronous I/O for file-backed HTTP/2 streams, by turning `FileHandle`s into (currently read-only) `StreamBase` instances, adding a generic mechanism to pipe from a `StreamBase` into another `StreamBase` (where currently the latter one needs to know that it wants to have data written to it, which is true for HTTP/2 streams).\r\n\r\nThis could probably land as it is, but I’ve marked it as WIP because the code probably isn’t quite documented yet and I want to investigate more into the performance of these changes:\r\n\r\nThe `http2/respond-with-fd` benchmark shows a ~30 % (update: down to more, like, 20 %) performance drop for me. At least part of that is  likely related to the way the benchmark is written: It performs tens or hundreds of reads for the same portion of the same short file in order to return data to the client. In particular, the kernel will always immediately have the data available and will never need to perform actual disk operations, so synchronous reads actually come with less overhead than asynchronous reads to begin with.\r\n\r\nIn some way, that’s realistic: A lot of the time, data *will* already be in the kernel cache. On the other hand, Node has committed to avoiding synchronous I/O, and we can’t really know whether disk I/O will be slow or fast in advance.\r\n\r\n(A lot of these assumptions are based on the fact than when I add a `nanosleep()` call with 1 ns duration – which obviously takes longer than 1 ns, but it appears to be enough – to the `pread64()` syscall, the 30 % drop turns into a whopping 100 % perf win).\r\n\r\nIn any case, I’d love to hear ideas and suggestions, both about the general approach and its performance here.\r\n\r\n/cc @nodejs/http2 @jasnell @mcollina @apapirovski @Fishrock123 \r\n\r\n---\r\n\r\nCommits:\r\n\r\n* http2: simplify timeout tracking\r\n    \r\n    There’s no need to reset the chunk counter for every write.\r\n\r\n* src: make `FileHandle` a (readonly) `StreamBase`\r\n    \r\n    This enables accessing files using a more standard pattern.\r\n    \r\n    Once some more refactoring has been performed on the other existing\r\n    `StreamBase` streams, this could also be used to implement `fs`\r\n    streams in a more standard manner.\r\n\r\n* src: give StreamBases the capability to ask for data\r\n    \r\n    Add a `OnStreamWantsWrite()` event that allows streams to\r\n    ask for more input data if they want some.\r\n\r\n* src: introduce native-layer stream piping\r\n    \r\n    Provide a way to create pipes between native `StreamBase` instances\r\n    that acts more directly than a `.pipe()` call would.\r\n\r\n* http2: use native pipe instead of synchronous I/O\r\n    \r\n    This resolves the issue of using synchronous I/O for\r\n    `respondWithFile()` and `respondWithFD()`, and enables\r\n    scenarios in which the underlying file does not need\r\n    to be a regular file.\r\n\r\n* http2: remove regular-file-only restriction\r\n    \r\n    Requiring `respondWithFile()` to only work with regular files\r\n    is an artificial restriction on Node’s side and has become unnecessary.\r\n    \r\n    Offsets or lengths cannot be specified for those files,\r\n    but that is an inherent property of other file types.\r\n\r\n\r\n##### Checklist\r\n<!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->\r\n\r\n- [x] `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes\r\n- [x] tests and/or benchmarks are included\r\n- [x] documentation is changed or added\r\n- [x] commit message follows [commit guidelines](https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines)\r\n\r\n##### Affected core subsystem(s)\r\n\r\nsrc,http2\r\n\r\n~~CI: https://ci.nodejs.org/job/node-test-commit/16439/~~\r\n~~CI: https://ci.nodejs.org/job/node-test-commit/16441/~~\r\nCI: https://ci.nodejs.org/job/node-test-commit/16447/"},"comment":{"url":"https://api.github.com/repos/nodejs/node/issues/comments/374001539","html_url":"https://github.com/nodejs/node/pull/18936#issuecomment-374001539","issue_url":"https://api.github.com/repos/nodejs/node/issues/18936","id":374001539,"user":{"login":"addaleax","id":899444,"avatar_url":"https://avatars2.githubusercontent.com/u/899444?v=4","gravatar_id":"","url":"https://api.github.com/users/addaleax","html_url":"https://github.com/addaleax","followers_url":"https://api.github.com/users/addaleax/followers","following_url":"https://api.github.com/users/addaleax/following{/other_user}","gists_url":"https://api.github.com/users/addaleax/gists{/gist_id}","starred_url":"https://api.github.com/users/addaleax/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/addaleax/subscriptions","organizations_url":"https://api.github.com/users/addaleax/orgs","repos_url":"https://api.github.com/users/addaleax/repos","events_url":"https://api.github.com/users/addaleax/events{/privacy}","received_events_url":"https://api.github.com/users/addaleax/received_events","type":"User","site_admin":false},"created_at":"2018-03-18T14:01:29Z","updated_at":"2018-03-18T14:01:29Z","author_association":"OWNER","body":"This depends on https://github.com/nodejs/node/pull/18297, so I don’t think we can backport it."}}
{"actor":{"display_login":"addaleax"}}