http: various performance improvements - #10558
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
why those changes? are they faster, or safer?
Sorry, something went wrong.
There was a problem hiding this comment.
It's more explicit and therefore at least a little safer and it's consistent with the other regexps we use in _http_server.js when searching header values that can be comma-separated lists.
I did not measure the performance of this particular change on its own.
Sorry, something went wrong.
There was a problem hiding this comment.
Is this giving any perf increase? The major benefit is to avoid the creation of CorkedRequest, which was introduced to leverage hidden classes and the like.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes. This is similar to what was done awhile back for process.nextTick() request objects, changed from new Foo() to plain object.
Sorry, something went wrong.
There was a problem hiding this comment.
clever! nice catch!
Sorry, something went wrong.
There was a problem hiding this comment.
we can probably reduce a bit the code footprint here by using:
{ chunk, encoding, callback: cb, next: null }Not sure if this functions was inlineable before and/or this change remove that.
Sorry, something went wrong.
There was a problem hiding this comment.
I thought about that, but wasn't sure if that would cause compatibility problems for readable-stream users or if the babel transforms took care of that or what...
Sorry, something went wrong.
There was a problem hiding this comment.
I think so, babel can/will take care of that.
Sorry, something went wrong.
There was a problem hiding this comment.
Changed.
Sorry, something went wrong.
There was a problem hiding this comment.
can you please update the build script in readable-stream when this lands too?
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
There was a problem hiding this comment.
This additional throw probably makes this PR semver-major?
Sorry, something went wrong.
There was a problem hiding this comment.
It shouldn't, I just had to move the check from the old _renderHeaders() to the appropriate call sites since that function no longer exists. I tried just moving it to _storeHeader() but that caused problems IIRC.
Sorry, something went wrong.
There was a problem hiding this comment.
As does this one
Sorry, something went wrong.
There was a problem hiding this comment.
It shouldn't, I just had to move the check from the old _renderHeaders() to the appropriate call sites since that function no longer exists. I tried just moving it to _storeHeader() but that caused problems IIRC.
Sorry, something went wrong.
There was a problem hiding this comment.
I surprisingly see Content-type and Content-length in the wild a ton. It may be useful to add those as well?
Sorry, something went wrong.
There was a problem hiding this comment.
I had to stop somewhere, plus you can only have a switch statement so large before the function gets deopt'ed (I think this may still be the case anyway).
Sorry, something went wrong.
There was a problem hiding this comment.
Is it not faster (and more elegant imho) to have a lookup object?
Sorry, something went wrong.
There was a problem hiding this comment.
From my testing it isn't faster, whether with a plain object or Map.
Sorry, something went wrong.
There was a problem hiding this comment.
is it safe to use instanceof here?
Sorry, something went wrong.
There was a problem hiding this comment.
It should be, since we're the ones storing header values in this._headers. That's also the reason why we can use for-in instead of Object.keys() (slight speedup there) when iterating over this._headers.
Sorry, something went wrong.
There was a problem hiding this comment.
should we not use similar logic as in matchKnownFields here?
Sorry, something went wrong.
There was a problem hiding this comment.
I don't recall specifically if I tested it in here or not, but in some places such functions didn't seem to perform as well for whatever reason.
Sorry, something went wrong.
There was a problem hiding this comment.
did this change actually make a perf impact?
Sorry, something went wrong.
There was a problem hiding this comment.
It wasn't about performance but easier readability and because I check if the loop was entered below by checking k.
Sorry, something went wrong.
There was a problem hiding this comment.
ah ok, that makes sense
Sorry, something went wrong.
There was a problem hiding this comment.
Can these be Maps instead? Also, let?
Sorry, something went wrong.
There was a problem hiding this comment.
AFAIK Map is still slow and http benchmarks already take a very long time as it is.
As far as converting to ES6 goes, that's for a separate PR I think. All I was doing in this part here was the changing style.
Sorry, something went wrong.
There was a problem hiding this comment.
AFAIK Map is still slow and http benchmarks already take a very long time as it is.
That is why we have https://github.com/nodejs/node/blob/master/benchmark/es/map-bench.js - tl;dr, it's faster.
Sorry, something went wrong.
There was a problem hiding this comment.
Map performance definitely appears to have improved significantly as of late.
Sorry, something went wrong.
There was a problem hiding this comment.
Right, but I seem to recall some issue with that benchmark. I think there was discussion in some issue/PR awhile ago about it?
Sorry, something went wrong.
|
@mscdex ... looks like this needs a rebase |
Sorry, something went wrong.
Sorry, something went wrong.
|
/cc @nodejs/ctc I'm now marking this as semver-major because I just realized I forgot to run citgm and I just found out that there are apparently users (at least on github) that are using res._headerNames without checking its value first and/or making assumptions about the property values on res._headers, both of which changed in this PR. So far the only instance on citgm where I ran into this is express, which uses the fresh module which makes assumptions about res._headers values. I will be submitting a PR to that project ASAP (EDIT: jshttp/fresh#20). @ChALkeR Could you find out how many other modules on npm may be affected? I really apologize for all of this.... |
Sorry, something went wrong.
|
I've now submitted a PR that should help resolve the main use cases for end users accessing ._headers in the first place: #10805 Since IIRC we don't have an official policy on undocumented, "private" (underscore-prefixed) properties yet, it would be good to hear what others think should be the way forward on this. Some ideas:
As far as the now removed ._headerNames goes, we could do like option 3 above and re-add it as a getter that results in a deprecation warning but still returns the original cased name now stored in ._headers. However I don't know if using getters like this would break anyone still or not (again, it's also an undocumented, "private" property, so how much do we care?) or at least to a lesser degree. Also, I am going to go out on a limb and say that ._headerNames usage is probably much less than ._headers usage in the wild. |
Sorry, something went wrong.
|
considering that fresh was downloaded 309,483 times in the last day, I think we should revert (quickly) and then re-evaluate. I would think that there are way too many people depending on older versions of express for us to revert this. |
Sorry, something went wrong.
PR-URL: nodejs#10558 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
By enforcing the statusCode to be an SMI, it helps a bit performance-wise when looking up the associated statusMessage. PR-URL: nodejs#10558 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
This commit implements two optimizations when working with headers: * Avoid having to explicitly "render" headers and separately store the original casing for header names. * Match special header names using a single regular expression instead of testing one regular expression per header name. PR-URL: nodejs#10558 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
PR-URL: nodejs#10558 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
PR-URL: nodejs#10558 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
PR-URL: nodejs#10558 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
Without this, the http benchmarker would report "strange output" if wrk or any other http client reported 0 requests per second, which is a valid result. PR-URL: nodejs#10558 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
This is similar to a change made awhile back for storing process.nextTick() requests. PR-URL: nodejs#10558 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
PR-URL: nodejs#10558 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
|
Any feedback from additional @nodejs/ctc members? Especially on any of the solutions I've proposed? |
Sorry, something went wrong.
That sounds like a nice enough solution. Warn in v8.x and remove in v9.x? |
Sorry, something went wrong.
|
So to give a little detail here from Express side, the ._headers usage in Express is mainly used in the conditional response logic, and it's location is at the heart of the Express code paths for almost every version of Express ever released so far. I would venture to guess almost every Express app out there is running over this code path. @mscdex made some PRs and we are working to iron out some edge cases, but when Express does release a version that does not reference ._headers there is certainly many years of versions out there that do. I can say from user support that it is very common for people not to be using the latest version of Express, even when the latest version has been out for over half a year, but I don't really know what percentage of those users end up on a new version of Express. My thoughts are that if 8.x is going to become the next LTS, it would be really nice if the warning / removal started in a non-LTS version based on the rate it seems people upgrade Express. From the past, even a warning will end up having a lot of fallout from confused users all over the place, so just brainstorming how can we have the best chance for the change to go out unnoticed by the vast majority of the users. |
Sorry, something went wrong.
|
Alright, I have proposed a getter-based solution in #10941. |
Sorry, something went wrong.
|
I am still +1 for reverting fwiw. I think we need to investigate better how this impacts the ecosystem especially considering that citgm is broken with this change... |
Sorry, something went wrong.
|
@evanlucas If at least one other @nodejs/ctc member LGTM's #10941 then it shouldn't be an issue anymore. |
Sorry, something went wrong.
These commits bring 3-11% performance increase (this is on top of the performance improvements made recently in #10445, #10443, and #6533). The performance increases are greater when you start setting/adding more headers than the http benchmarks currently use (just 2 currently).
When using the 'simple' http benchmark, I had to reduce the http client benchmarker duration by half (5 seconds) and reduce the number of runs to 10 (from the default of 30) to get the results back in a reasonable amount of time, especially with the newly added benchmark parameter. This allowed me to finish benchmarking in a little over 4.5 hours, whereas before with the original duration and 30 runs it was still running after 18 hours (when it technically should have finished -- even taking into account overhead since a single run doesn't last exactly the duration specified).
With that being said, here are the results with those modified benchmarking settings:
DetailsWhen adding 3 custom headers in the 'setHeaderWH' case for example, you can see a greater increase:
Details/cc @nodejs/http
/cc @nodejs/streams for stream changes
CI: https://ci.nodejs.org/job/node-test-pull-request/5651/
CI: https://ci.nodejs.org/job/node-test-pull-request/5652/
Checklist
Affected core subsystem(s)