performance regression against master

[mcollina]

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:

[targos]

@nodejs/v8

[ofrobots]

Can you also try with --nountrusted_code_mitigations (link)?

[mcollina]

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):

benchThrough2*10000: 2667.087ms
benchPassThrough*10000: 2539.030ms
benchThrough*10000: 598.221ms
benchSyncThrough*10000: 363.399ms
benchThrough2*10000: 2672.474ms
benchPassThrough*10000: 2672.423ms
benchThrough*10000: 611.136ms
benchSyncThrough*10000: 343.683ms

with --nountrusted_code_mitigations:

$ ./node --nountrusted_code_mitigations ~/Repositories/syncthrough/benchmarks/basic.js
benchThrough2*10000: 2876.624ms
benchPassThrough*10000: 2734.464ms
benchThrough*10000: 634.742ms
benchSyncThrough*10000: 384.965ms
benchThrough2*10000: 2714.740ms
benchPassThrough*10000: 2733.384ms
benchThrough*10000: 621.585ms
benchSyncThrough*10000: 351.250ms

without --nountrusted_code_mitigations:

$ ./node ~/Repositories/syncthrough/benchmarks/basic.js
benchThrough2*10000: 3067.916ms
benchPassThrough*10000: 2926.186ms
benchThrough*10000: 896.684ms
benchSyncThrough*10000: 466.958ms
benchThrough2*10000: 2969.392ms
benchPassThrough*10000: 2955.923ms
benchThrough*10000: 853.613ms
benchSyncThrough*10000: 434.896ms

Essentially yes, --nountrusted_code_mitigations  fixes it. What does that do?

(With the new release I can't replicate the effect on http, which is good)

[ebraminio]

--nountrusted_code_mitigations fixes it. What does that do?

Spectre and Meltdown related mitigations, not needed for usual use of node.js AFAIK.

[mcollina]

oh nice. Can we flip the switch to have --untrusted_code_mitigations instead?
Or is this already happening on master?

[hashseed]

Untrusted code mitigations are on by default in V8. Having them on regresses performance. --no-untrusted-code-mitigations disables them and fixes the regression.

@mcollina you said earlier that the flag does not work for you. Did you find out why?

[mcollina]

@mcollina you said earlier that the flag does not work for you. Did you find out why?

@hashseed I used an older binary, I wrongly guessed that the files were sorted, but they weren't.

Untrusted code mitigations are on by default in V8. Having them on regresses performance. --no-untrusted-code-mitigations disables them and fixes the regression.

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]

Yes. The best way to do this is to add GYP configs to pass DISABLE_UNTRUSTED_CODE_MITIGATIONS as C++ define, to mirror this GN config.

[devsnek]

after we set DISABLE_UNTRUSTED_CODE_MITIGATIONS, can users still do something like --enable-untrusted-code-mitigations

[mcollina]

@hashseed Would this be needed on V8 6.6? Should we do this change after nodejs/node#19201?

[hashseed]

after we set DISABLE_UNTRUSTED_CODE_MITIGATIONS, can users still do something like --enable-untrusted-code-mitigations

Yes and no. It sets the default value for the flag, so you can still use --untrusted-code-mitigations to 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.

@hashseed Would this be needed on V8 6.6? Should we do this change after nodejs/node#19201?

Ideally as part of the PR to update V8 to 6.6, yes.

[targos]

@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]

Yes, that sounds good.

[bmeurer]

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]

This can be closed then