Skip to content
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

The performance gap between node16 and node21 changes as the n of the benchmark changes #50571

Closed
Septa2112 opened this issue Nov 6, 2023 · 8 comments · Fixed by #50585, #50868, #51408, #51416 or #51419
Labels
benchmark Issues and PRs related to the benchmark subsystem. good first issue Issues that are suitable for first-time contributors. performance Issues and PRs related to the performance of Node.js.

Comments

@Septa2112
Copy link
Contributor

Septa2112 commented Nov 6, 2023

Version

node 16.x and node 21.1

Platform

Linux wpe-icx 6.2.0-26-generic #26~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Thu Jul 13 16:27:29 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

I tested node-benchmark es/string-concatenations.js with node 21.1 and node 16.x.

When n is the default 1e3, the score difference between the two version reaches 50%.

But when n is set to 5000 or 10000 or other larger numbers, there is no performance gap between the two versions.

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

Expected

  • When I adjust n to 5e3 or 1e4, the score difference between the two versions should remain around 50%.

Reason

  • I think the number of n has not changed much, and the performance gap between node 16.x and node 21.1 should not change so much too.

What do you see instead?

  • n = 1e3 (default number)
NODE 16.x  es/string-concatenations.js mode="multi-join" n=1000: 2,275,198.9661495895
NODE 21.1  es/string-concatenations.js mode="multi-join" n=1000: 975,159.7555469525
  • n = 5e3
NODE 16.x  es/string-concatenations.js mode="multi-join" n=5000: 1,972,248.1025001006
NODE 21.1  es/string-concatenations.js mode="multi-join" n=5000: 1,983,125.977681107
  • n = 1e4
NODE 16.x  es/string-concatenations.js mode="multi-join" n=10000: 2,678,989.303063612
NODE 21.1  es/string-concatenations.js mode="multi-join" n=10000: 2,739,590.924283186

Additional information

I'm not sure whether this problem can be counted as an issue. Or is this a pre-existing problem?

@marco-ippolito marco-ippolito added the performance Issues and PRs related to the performance of Node.js. label Nov 6, 2023
@H4ad
Copy link
Member

H4ad commented Nov 6, 2023

Probably is just the V8 JIT kick-in and the start of the optimization in that code, maybe the behavior had changed and now they didn't optimize aggressively initially and only started the optimization after more iterations than before.

But this can have a lot of reasons, all of them are related to V8 than Node.js itself.

But you found an interesting case that we can increase the number of iterations to be more precise, are you willing to create a PR to increase that number? Just enough that still be fast but avoid this behavior.

@H4ad H4ad added good first issue Issues that are suitable for first-time contributors. benchmark Issues and PRs related to the benchmark subsystem. labels Nov 6, 2023
Septa2112 added a commit to Septa2112/node that referenced this issue Nov 7, 2023
Increase the number of iterations from `1e3` to `1e6`
to avoid the test performance gap caused by inactive
V8 optimization caused by too few iterations.

Fixes: nodejs#50571
@Septa2112
Copy link
Contributor Author

Thanks. I have submitted a PR #50585 to increase the number of iterations in benchmark/es/string-concatenations.js.

I plan to test other benchmarks later to see if similar situations exist. If so, I will modify all and create a unified PR.

I wonder if this is acceptable?

@H4ad
Copy link
Member

H4ad commented Nov 7, 2023

@Septa2112 It would be highly appreciated if you did that!

I suggest you create a commit for each benchmark change.

@Septa2112
Copy link
Contributor Author

BTW, I tried to analyze the cause of this phenomenon based on your tips. And I run the benchmark with --print-opt-code to see if node generated opt code.

I found that when iterations is default 1e3, neither node-16.x nor node-21.1 generated opt code.

When n increases to 1e5 or larger, I can get the opt code of the two versions of node. Just like:

......
--- Optimized code ---
optimization_id = 0
source_position = 321
kind = TURBOFAN
name = main
stack_slots = 26
compiler = turbofan
address = 0x7f141c054541

Instructions (size = 1228)
0x7f141c0545a0     0  488b59c0           REX.W movq rbx,[rcx-0x40]
0x7f141c0545a4     4  f6430f01           testb [rbx+0xf],0x1
......

Does this mean that the performance gap between node16 and node21 is not a problem caused by the inactive v8 JIT optimization because neither of them generated opt code when iterations is 1e3?

Please correct me if I say something wrong.

@H4ad
Copy link
Member

H4ad commented Nov 7, 2023

@Septa2112 You are right, at least on 1e3, the slowdown is not caused by V8 JIT.

I used the following code:

let string;

const n = 1e3;
const str = 'abc';
const num = 123;

const start = performance.now();

for (let i = 0; i < n; i++)
  string = ['...', str, ', ', num, ', ', str, ', ', num, '.'].join('');

const end = performance.now();

console.log(`Diff: ${end - start}ms`);
console.log(string);

For 1e3:

  • 16.20.1: 0.2531019998714328ms
  • 21.1.0: 0.24994500004686415ms

For `1e6:

  • 16.20.1: 130.60445600003004ms
  • 21.1.0: 123.54395300010219ms

Using 1e6 and --print-opt-code, we have the following comparison:

  • 16.20.1: Instructions (size = 1252)
  • 21.1.0: Instructions (size = 1112)

Is slower without optimizations but is faster faster the optimizations.

@Septa2112
Copy link
Contributor Author

@H4ad Thanks for your explanation!

Now I find that some other cases may have similar problems. Should I open a new issue? Or update these cases directly in the current issue and create new PRs directly?

@H4ad
Copy link
Member

H4ad commented Nov 8, 2023

Create new PRs and just reference this issue as the starting point to looking for those fixes.

nodejs-github-bot pushed a commit that referenced this issue Nov 9, 2023
Increase the number of iterations from `1e3` to `1e6`
to avoid the test performance gap caused by inactive
V8 optimization caused by too few iterations.

Fixes: #50571
PR-URL: #50585
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
targos pushed a commit that referenced this issue Nov 11, 2023
Increase the number of iterations from `1e3` to `1e6`
to avoid the test performance gap caused by inactive
V8 optimization caused by too few iterations.

Fixes: #50571
PR-URL: #50585
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Septa2112 added a commit to Septa2112/node that referenced this issue Nov 13, 2023
Increase the number of iterations from 1e5 to 5e6
to avoid the test performance gap caused by inactive
V8 optimization caused by insufficient number of iterations

Refs: nodejs#50571
targos pushed a commit that referenced this issue Nov 14, 2023
Increase the number of iterations from `1e3` to `1e6`
to avoid the test performance gap caused by inactive
V8 optimization caused by too few iterations.

Fixes: #50571
PR-URL: #50585
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
@Septa2112
Copy link
Contributor Author

Septa2112 commented Nov 17, 2023

@H4ad Hi, I have submitted a new related PR #50698 for several days but no one has approved it yet. Can you help me? Thanks!

lucshi pushed a commit to lucshi/node that referenced this issue Nov 23, 2023
lucshi pushed a commit to lucshi/node that referenced this issue Nov 23, 2023
Increase the iteration number from 500 to 2500 to make time consumption
of crypto operation dominate the benchmark running.

Fixes: nodejs#50571
lucshi pushed a commit to lucshi/node that referenced this issue Nov 23, 2023
Increase the iteration number from 5000 to 500000 to make time
consumption of crypto operation dominate the benchmark running.

Fixes: nodejs#50571
lucshi pushed a commit to lucshi/node that referenced this issue Nov 23, 2023
Increase the 500 iteration to 50000 to reflect the real ciphers perf.
The single iteration n=1 can not make the score reasonable.

Fixes nodejs#50571
lucshi pushed a commit to lucshi/node that referenced this issue Nov 23, 2023
Increase the iteration number from 1000 to 10000 to get a reasonble
score. The updated score has up to ~60% improvement.

Fixes: nodejs#50571
lucshi pushed a commit to lucshi/node that referenced this issue Nov 23, 2023
Increase iteration value from 1000 to 10000 to reflect real performance
of randomBytes generation. The randomInt has no need to apply this.

Fixes:nodejs#50571
marco-ippolito pushed a commit that referenced this issue Feb 29, 2024
marco-ippolito pushed a commit that referenced this issue Feb 29, 2024
Fixes: #50571
PR-URL: #51419
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
lucshi pushed a commit to lucshi/node that referenced this issue Mar 1, 2024
nodejs-github-bot pushed a commit that referenced this issue Mar 4, 2024
nodejs-github-bot pushed a commit that referenced this issue Mar 4, 2024
storage-getstore-nested-resources.js

Fixes: #50571
PR-URL: #51420
Reviewed-By: Chengzhong Wu <[email protected]>
targos pushed a commit that referenced this issue Mar 7, 2024
targos pushed a commit that referenced this issue Mar 7, 2024
storage-getstore-nested-resources.js

Fixes: #50571
PR-URL: #51420
Reviewed-By: Chengzhong Wu <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
PR-URL: #50651
Refs: #50571
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #50571
PR-URL: #50863
Refs: #50571
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #50571
PR-URL: #50866
Refs: #50571
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #50571
PR-URL: #50932
Refs: #50571
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #50571
PR-URL: #50933
Refs: #50571
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #50571
PR-URL: #50934
Refs: #50571
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #50571
PR-URL: #50937
Refs: #50571
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #50571
PR-URL: #50938
Refs: #50571
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #50571
PR-URL: #50868
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #50571
PR-URL: #50929
Refs: #50571
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #50571
PR-URL: #50869
Refs: #50571
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #50571
PR-URL: #51419
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
richardlau pushed a commit that referenced this issue Mar 25, 2024
storage-getstore-nested-resources.js

Fixes: #50571
PR-URL: #51420
Reviewed-By: Chengzhong Wu <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #50571
PR-URL: #51419
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
richardlau pushed a commit that referenced this issue Mar 25, 2024
richardlau pushed a commit that referenced this issue Mar 25, 2024
storage-getstore-nested-resources.js

Fixes: #50571
PR-URL: #51420
Reviewed-By: Chengzhong Wu <[email protected]>
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Mar 26, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Mar 26, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Mar 26, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Mar 26, 2024
storage-getstore-nested-resources.js

Fixes: nodejs#50571
PR-URL: nodejs#51420
Reviewed-By: Chengzhong Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment