-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
http: significant performance regression on master #37937
Comments
cc @ronag |
I think that the warning messages originated here: #36816 |
I think so as well. |
I will take a look as soon as I can |
I'm looking into it as well! |
I will have time tonight. Please keep me posted if you find anything. |
A couple of notes: #36816 is the cause of the warnings and likely memory leak. Going to the parent commit has a minor performance benefit:
The majority performance regression is present in v15 as well, so it's unrelated. |
Apparently the majority of the regression happened in the v15 cycle. v15.0.0: 77k req/sec (median) |
I will sort this. |
Are you bisecting? |
@mcollina: Any idea how to repro without using autocannon? I tried: 'use strict';
const common = require('../common');
const http = require('http');
const Countdown = require('../common/countdown');
const NUM_REQ = 128
const agent = new http.Agent({ keepAlive: true });
const countdown = new Countdown(NUM_REQ, () => server.close());
const server = http.createServer(common.mustCall(function(req, res) {
res.setHeader('content-type', 'application/json; charset=utf-8')
res.end(JSON.stringify({ hello: 'world' }))
}, NUM_REQ)).listen(0, function() {
for (let i = 0; i < NUM_REQ; ++i) {
http.request({
port: server.address().port,
agent,
method: 'GET'
}, function(res) {
res.resume();
res.on('end', () => {
countdown.dec();
})
}).end();
}
});
process.on('warning', common.mustNotCall()); |
I've done a I'm doing a |
I think you need to use |
And between which versions did it occur? |
So far I have identified these commits as problematic: There are probably more. I'll start to suspect the other primordials change. On df08d52, I benchmarked this extensively and I did not see a regression at the time. Also it did not have the same worsening effect on v14 instead of master. |
Is it the |
Yes it is. |
cc @nodejs/tsc |
I have identified the three offending commits that causes these regressions:
I plan to issue a revert for 0b6d307 and e2f5bb7 and work on a faster implementation of df08d52 as it's a security fix. cc @nodejs/http |
e2f5bb7 has 2 problems then:
What's the timeframe where we need this fixed? If there is time I'd like to have a go at trying to resolve these. |
Before v16 comes out. It can either be yanked from the release or reverted on master. I prefer to revert as we are already close to the deadline for semver-majors. |
Alright. I'll have another go at it for v17? Assuming we agree that the core idea behind the PR is still considered good. |
I still think it's a good idea. I'll add a test to prevent the regression in the future. |
A test with pipelining would be nice to sort the listener leak regression. |
I have trouble to get consistent results on my computer with the provided benchmark. Every time I execute autocannon, I get a very different result, up to 10% different from another one. |
Exporting a variable that will be mutated later doesn't work. Refs: #37937 PR-URL: #37966 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
This reverts commit 2da3611. PR-URL: #37964 Refs: #37937 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #37964 Refs: #37937 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
I think this can be closed now? From a quick glance at the CPU profile, it looks like the biggest bottleneck on master is now |
I think there is still some gaps. Here are this morning tests. master:
node v14.16.0:
I plan to do a bit more work on this area and try to bring it back to the same throughput. |
I have identified a significant regression between v15.5.1 and v15.6.0. Node v15.5.1
Node v15.6.0
|
Nevermind, this was the debuglog that @targos fixed. |
@mcollina excellent work! |
I've got the fix already identified, I'm just not writing any code today so I will do that first thing Monday morning and have the PR open soon after |
Do these things not show up when running a cpu profile? Seems a bit unfortunate that we need to git bisect to identify new or old performance bottlenecks? |
They show up. However git bisecting is actually simpler because you do not have to code hypothetical fixes. The hrtime showed up. It's far down from the main bottleneck, but it was a key difference on an hot path. |
And I did run perf tests on this one change. I just think some of the other issues were masking the perf hit one the hrtime change making it far less obvious. Fortunately, it's a quick fix |
Exporting a variable that will be mutated later doesn't work. Refs: #37937 PR-URL: #37966 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Signed-off-by: James M Snell <[email protected]> Refs: nodejs#37937 Refs: nodejs#37136
Only call into hrtime if there's an observer Also, fix up some previously missed changes from the original refactor Signed-off-by: James M Snell <[email protected]> Refs: nodejs#37937 Refs: nodejs#37136
Only call into hrtime if there's an observer Also, fix up some previously missed changes from the original refactor Signed-off-by: James M Snell <[email protected]> Refs: nodejs#37937 Refs: nodejs#37136
Only call into hrtime if there's an observer Also, fix up some previously missed changes from the original refactor Signed-off-by: James M Snell <[email protected]> Refs: #37937 Refs: #37136 PR-URL: #38110 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
Here is another one: #38245. I have one more coming :). |
#38246 include my last findings. |
With the latest PRs having landed, the HTTP throughput of the upcoming v16 is on par with the one of the latest v14. I think this is a milestone and I'll celebrate to close this issue and possibly open a fresh one with other optimizations that we might want to do here. |
What steps will reproduce the bug?
run:
and then:
on
v14.16
this produces:on
master
:On master it also produces a significant amount of warnings:
Update as of 2021/3/29 bisect from head a9cdeed:
The text was updated successfully, but these errors were encountered: