http: server code refactor #6533
Conversation
mscdex
added
wip
|
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. |
|
/cc @nodejs/collaborators @nodejs/streams Any comments, especially on the controversial changes? |
| const req = parser.incoming; | ||
| debug('SERVER upgrade or connect', req.method); | ||
|
|
||
| if (!d) |
There was a problem hiding this comment.
I'd find it more readable if this was on the same line or with {}s
|
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?
|
|
@benjamingr I'm not sure what in particular (in 1e824be) you're looking for in the commit message? |
| } | ||
| return valid; | ||
| return true; | ||
| } |
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.
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.
|
This is amazing work @mscdex! Some comments on your questions:
No problem,
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 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. |
|
@mcollina Well actually I haven't done any benchmarks yet, but I'm not sure if adding the overhead of |
Not really, if
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 |
|
@mcollina RE: |
Yes! Before #4354 it was reallocated whenever we did Again, I really think we should consider splitting this into 2 PRs, one stream-related and one HTTP related. It will simplify benchmarking/reviewing. |
rvagg
force-pushed
the
master
branch
2 times, most recently
from
c133999 to
83c7a88
Compare
mscdex
force-pushed
the
http-server-refactor
branch
from
3d6c0b7 to
9e2cf69
Compare
|
Alright, I've completely redone this PR now. Things to note:
CI: https://ci.nodejs.org/job/node-test-pull-request/5493/ |
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.
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.
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.
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:
readable-streammodule in particular here, which would see problems with older versions of node (e.g. v0.10), unless these changes were specifically not pulled in there (is this even feasible, despite older node versions going unmaintained at the end of this year?).write()callbacks as a second argumentargumentsinside callbacks passed towrite()write()callback used inOutgoingMessage.prototype.end(), since writable streams do not call theirwrite()callbacks within the context of the stream (e.g. they usecb()instead ofcb.call(stream)./cc @nodejs/http