performance regression against master #40
Description
I got a performance regression from master to canary.
https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js
In master:
benchThrough2*10000: 2727.827ms
benchPassThrough*10000: 2488.877ms
benchThrough*10000: 602.869ms
benchSyncThrough*10000: 376.744ms
benchThrough2*10000: 2548.351ms
benchPassThrough*10000: 2513.266ms
benchThrough*10000: 579.024ms
benchSyncThrough*10000: 320.260ms
On canary:
benchThrough2*10000: 7532.033ms
benchPassThrough*10000: 9418.588ms
benchThrough*10000: 676.452ms
benchSyncThrough*10000: 431.320ms
benchThrough2*10000: 3467.997ms
benchPassThrough*10000: 3448.511ms
benchThrough*10000: 638.495ms
benchSyncThrough*10000: 402.225ms
On node 9.7.0:
benchThrough2*10000: 2949.809ms
benchPassThrough*10000: 2758.394ms
benchThrough*10000: 586.657ms
benchSyncThrough*10000: 444.994ms
benchThrough2*10000: 2985.265ms
benchPassThrough*10000: 2875.148ms
benchThrough*10000: 563.383ms
benchSyncThrough*10000: 345.953ms
On node 8.10.0:
benchThrough2*10000: 3153.932ms
benchPassThrough*10000: 3223.773ms
benchThrough*10000: 833.032ms
benchSyncThrough*10000: 525.098ms
benchThrough2*10000: 3631.106ms
benchPassThrough*10000: 3148.411ms
benchThrough*10000: 874.470ms
benchSyncThrough*10000: 365.787ms
I can see a similar regression on http as well, if we run:
'use strict'
const server = require('http').createServer(function (req, res) {
res.setHeader('Content-Type', 'application/json')
res.end(JSON.stringify({ hello: 'world' }))
})
server.listen(3000)And then we load test with npx autocannon -c 100 -d 5 -p 10 localhost:3000 twice and get the second number:
- canary 130k requests in 5s, 19.4 MB read
- master 151k requests in 5s, 22.4 MB read
- 8.10 94k requests in 5s, 14 MB read
- 9.7.0 136k requests in 5s, 20.3 MB read
- Version: node-v10.0.0-v8-canary20171101b81802a96b-darwin-x64 vs 1a5ec83
- Platform: mac os x
- Subsystem:
Activity
targos commentedon Mar 7, 2018
@nodejs/v8
ofrobots commentedon Mar 7, 2018
Can you also try with
--nountrusted_code_mitigations(link)?mcollina commentedon Mar 7, 2018
I downloaded an old canary build, I'm now on node-v10.0.0-v8-canary20180306870146ff63-darwin-x64.
master (not sure why but I can't replicate the numbers I had this morning):
with
--nountrusted_code_mitigations:without
--nountrusted_code_mitigations:Essentially yes,
--nountrusted_code_mitigationsfixes it. What does that do?(With the new release I can't replicate the effect on http, which is good)
ebraminio commentedon Mar 7, 2018
Spectre and Meltdown related mitigations, not needed for usual use of node.js AFAIK.
mcollina commentedon Mar 7, 2018
oh nice. Can we flip the switch to have
--untrusted_code_mitigationsinstead?Or is this already happening on master?
hashseed commentedon Mar 7, 2018
Untrusted code mitigations are on by default in V8. Having them on regresses performance.
--no-untrusted-code-mitigationsdisables them and fixes the regression.@mcollina you said earlier that the flag does not work for you. Did you find out why?
mcollina commentedon Mar 7, 2018
@hashseed I used an older binary, I wrongly guessed that the files were sorted, but they weren't.
What I mean is, can we flip this behavior in Node.js against V8? It seems they are not part of Node.js threat model, so we can live without them and enjoy better performance.
hashseed commentedon Mar 7, 2018
Yes. The best way to do this is to add GYP configs to pass
DISABLE_UNTRUSTED_CODE_MITIGATIONSas C++ define, to mirror this GN config.devsnek commentedon Mar 7, 2018
after we set DISABLE_UNTRUSTED_CODE_MITIGATIONS, can users still do something like
--enable-untrusted-code-mitigationsmcollina commentedon Mar 7, 2018
@hashseed Would this be needed on V8 6.6? Should we do this change after nodejs/node#19201?
hashseed commentedon Mar 7, 2018
Yes and no. It sets the default value for the flag, so you can still use
--untrusted-code-mitigationsto enable it. However, V8 compiles some of its builtin code at build time to include in the startup snapshot. Once included, the runtime flag won't be able to change it anymore. These builtin code are also affected by mitigations. Though I expect that the performance impact to be less significant. But the fact these builtins do not include mitigations may be a security risk if that's an issue.Ideally as part of the PR to update V8 to 6.6, yes.
targos commentedon Mar 7, 2018
@hashseed knowing what you just said, should we instead compile with untrusted code mitigations and disable them by default when Node starts (with
v8.setFlagsFromString)?hashseed commentedon Mar 7, 2018
Yes, that sounds good.
bmeurer commentedon Mar 8, 2018
You'll probably need this on 6.5 as well. I'm not so eager to have untrusted code mitigations build into the Node snapshot. I think it's better to consistently disable them via the GN/GYP flag. If someone needs a Node with these mitigations, he/she should build a proper binary. We're already stretching the test matrix quite a bit here...
mcollina commentedon Mar 16, 2018
This can be closed then