http: server code refactor - #6533
Conversation
Sorry, something went wrong.
|
First off, thank you for doing this. This module needed some love badly. Glad to see it getting some attention. The readable-stream changes are certainly just a bit concerning. Let's see what @nodejs/streams has to say. |
Sorry, something went wrong.
|
/cc @nodejs/collaborators @nodejs/streams Any comments, especially on the controversial changes? |
Sorry, something went wrong.
There was a problem hiding this comment.
I'd find it more readable if this was on the same line or with {}s
Sorry, something went wrong.
|
I've attempted reading it but I'm not really sure - would you mind underlying the changes I should be looking at and explaining them? Alternatively, I would split the commits of things like var -> const and naming function expressions to a separate commit since it makes it hard to me to distil the parts that changed logic vs. cosmetic changes. I'm aware that the rest of core might not feel that way. nevermind I see you already did this and I was fooled by GH.
|
Sorry, something went wrong.
|
@benjamingr I'm not sure what in particular (in 1e824be) you're looking for in the commit message? |
Sorry, something went wrong.
There was a problem hiding this comment.
There might be a slight benefit in having a single return point. Not sure if it is visibile here or not.
Sorry, something went wrong.
There was a problem hiding this comment.
Why?
Sorry, something went wrong.
There was a problem hiding this comment.
It's very low level and definitely valid for some old version of V8. It is due to how v8 "compiles" this code, basically having a single return statement allow V8 to produce "faster" code.
Not sure if still applies.. @trevnorris can definitely explain it better than I do.
Sorry, something went wrong.
|
This is amazing work @mscdex! Some comments on your questions:
No problem, readable-stream is already transpiled (from nodejs/readable-stream#186, 2.0.5). Go ahead and use all of the ES6 feature you like. cc @calvinmetcalf.
This is not correct. See the technique I used in https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L394, in particular https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L511-L530. Yes, it is really really ugly but it gets the job done. A module that does the same trick is https://github.com/mcollina/reusify. The overhead slightly reduce the benefit, but it's better than an API change. I would rather prefer to change the interface so that cb is called within the context of the stream. This might involve some more work, I will encourage you to do two PRs, one with the stream change and one with the HTTP one. It's a bit too much to reason about here. It might also affect fs and net differently. On the other end, I do not see a compatibility problem in either case. I need this feature a gazillion of other places, and I do not see any way this could break as currently it's not set. I do not see a compatibility issue because this is used internally, and currently there is not a public API to run an HTTP server on top of any stream. |
Sorry, something went wrong.
|
@mcollina Well actually CorkedRequest.finish() is another method that continually gets recompiled/reoptimized because it's not a prototype method, but that's a separate issue ;-) I haven't done any benchmarks yet, but I'm not sure if adding the overhead of cb.call() (vs cb()) is a worthwhile tradeoff just to benefit the http module, since it will slow down all other stream users? |
Sorry, something went wrong.
Not really, if clearBuffer is not called multiple times synchronously, only two of those for each stream should exists. This is 99,999% of the use case (uncork in HTTP is wrappend in process.nextTick), so that should get optimized pretty heavily. If not, then we should have a look.
It's already everywhere in all the EE interface, and for each chunk it is used quite a bit. I think it will not cause a significant decrease. Note that most users do not use the callback on write, but they rely on pipe, which does not pass a callback. |
Sorry, something went wrong.
|
@mcollina RE: CorkedRequest.finish(), it gets recompiled/reoptimized for every http connection. This is very noticeable when you run the http benchmarks and you have --trace-opt, etc. turned on. |
Sorry, something went wrong.
Yes! Before #4354 it was reallocated whenever we did _writev. Again, I really think we should consider splitting this into 2 PRs, one stream-related and one HTTP related. It will simplify benchmarking/reviewing. |
Sorry, something went wrong.
|
Alright, I've completely redone this PR now. Things to note:
CI: https://ci.nodejs.org/job/node-test-pull-request/5493/ |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
Can you submit the changes to Streams into a different PR? I do not think it's the right time to merge those, and we should do a more overhaul on them when the next LTS comes out.
At the moment, we do not have the infrastructure to pull in or skip specific commits.
Sorry, something went wrong.
PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The new table-based lookups perform significantly better for the common cases (checking latin1 characters). PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Since at least V8 5.4, using function.bind() is now fast enough to use to avoid recompiling/reoptimizing the same anonymous functions. These changes especially impact http servers. PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This commit uses instanceof instead of Array.isArray() for faster type checking and avoids calling Object.keys() when the headers are stored as a 2D array instead of a plain object. PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Notable changes: * buffer: - Improve performance of Buffer allocation by ~11% (Brian White) #10443 - Improve performance of Buffer.from() by ~50% (Brian White) #10443 * events: Improve performance of EventEmitter.once() by ~27% (Brian White) #10445 * http: Improve performance of http server by ~7% (Brian White) #6533
PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The new table-based lookups perform significantly better for the common cases (checking latin1 characters). PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Since at least V8 5.4, using function.bind() is now fast enough to use to avoid recompiling/reoptimizing the same anonymous functions. These changes especially impact http servers. PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This commit uses instanceof instead of Array.isArray() for faster type checking and avoids calling Object.keys() when the headers are stored as a 2D array instead of a plain object. PR-URL: #6533 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Notable changes: * buffer: - Improve performance of Buffer allocation by ~11% (Brian White) #10443 - Improve performance of Buffer.from() by ~50% (Brian White) #10443 * events: Improve performance of EventEmitter.once() by ~27% (Brian White) #10445 * fs: Allow passing Uint8Array to fs methods where Buffers are supported. (Anna Henningsen) #10382 * http: Improve performance of http server by ~7% (Brian White) #6533 * npm: Upgrade to v4.0.5 (Kat Marchán) #10330 PR-URL: #10589
Notable changes: * buffer: - Improve performance of Buffer allocation by ~11% (Brian White) #10443 - Improve performance of Buffer.from() by ~50% (Brian White) #10443 * events: Improve performance of EventEmitter.once() by ~27% (Brian White) #10445 * fs: Allow passing Uint8Array to fs methods where Buffers are supported. (Anna Henningsen) #10382 * http: Improve performance of http server by ~7% (Brian White) #6533 * npm: Upgrade to v4.0.5 (Kat Marchán) #10330 PR-URL: #10589
| 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, | ||
| 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, | ||
| 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 // ... 255 | ||
| ]; |
There was a problem hiding this comment.
do you have any reference mat'l on the choice of int vs bool here? just interested.
Sorry, something went wrong.
There was a problem hiding this comment.
Performance-wise? Not really. At the very least it's more compact. I wouldn't be surprised though if V8 internally treated booleans as SMIs with values of 0 or 1.
Sorry, something went wrong.
Notable changes:
* buffer:
- Improve performance of Buffer allocation by ~11% (Brian White) nodejs/node#10443
- Improve performance of Buffer.from() by ~50% (Brian White) nodejs/node#10443
* events: Improve performance of EventEmitter.once() by ~27% (Brian White) nodejs/node#10445
* fs: Allow passing Uint8Array to fs methods where Buffers are supported. (Anna Henningsen) nodejs/node#10382
* http: Improve performance of http server by ~7% (Brian White) nodejs/node#6533
* npm: Upgrade to v4.0.5 (Kat Marchán) nodejs/node#10330
PR-URL: nodejs/node#10589
Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Checklist
Affected core subsystem(s)
Description of change
This PR consists of a refactor of http server-related code as well as various cleanups and minor optimizations in the http and writable stream modules.
Here are some benchmark results with these changes:
Probably the more controversial changes here would be the ones in the writable stream module because of:
/cc @nodejs/http