Skip to content

Conversation

Septa2112
Copy link
Contributor

@Septa2112 Septa2112 commented Dec 4, 2023

Improve performance of areSimilarFloatArrays by using primordial.

  • Platform: Linux

How find the issue

When I test performance between node-16.x and node-22-pre, I found performance regression in benchmark/assert/deepequal-typedarrays.js. Old binary is node-16.x and the new is node-22-pre

Original result of node-16.x and node-22-pre

assert/deepequal-typedarrays.js len=100 method='deepEqual' strict=0 n=500 type='Float32Array'            ***    -22.09 %       ±1.61% ±2.16% ±2.84%
assert/deepequal-typedarrays.js len=100 method='notDeepEqual' strict=0 n=500 type='Float32Array'         ***    -13.65 %       ±2.85% ±3.83% ±5.04%
assert/deepequal-typedarrays.js len=5000 method='deepEqual' strict=0 n=500 type='Float32Array'           ***    -56.36 %       ±0.80% ±1.08% ±1.42%
assert/deepequal-typedarrays.js len=5000 method='notDeepEqual' strict=0 n=500 type='Float32Array'        ***    -67.89 %       ±4.42% ±6.04% ±8.20%

How solved

I found the case benchmark/assert/deepequal-typedarrays.js will finally call function areSimilarFloatArrays(a, b) in lib/internal/util when strict=0 type='Float32Array'. The problem is similar with #50620. So I learned from the approach in #50621.

Results

Comparison between node_16.x and node-22-pre

I modified the code in main branch (v22.0.0-pre), and compare with node_16.x

assert/deepequal-typedarrays.js len=100 method='deepEqual' strict=0 n=500 type='Float32Array'            ***    -11.67 %       ±1.81% ±2.44% ±3.23%
assert/deepequal-typedarrays.js len=100 method='notDeepEqual' strict=0 n=500 type='Float32Array'         ***     -7.85 %       ±2.34% ±3.13% ±4.12%
assert/deepequal-typedarrays.js len=5000 method='deepEqual' strict=0 n=500 type='Float32Array'           ***     24.98 %       ±0.74% ±0.99% ±1.31%
assert/deepequal-typedarrays.js len=5000 method='notDeepEqual' strict=0 n=500 type='Float32Array'                -1.61 %       ±4.01% ±5.40% ±7.14%

Performance improvement on main(node-22-pre)

After the change, test on the latest main branch and get the results of performance comparison of node_22 before and after modifying the code.

assert/deepequal-typedarrays.js len=100 method='deepEqual' strict=0 n=500 type='Float32Array'            ***     11.33 %       ±1.89% ±2.53%  ±3.34%
assert/deepequal-typedarrays.js len=100 method='notDeepEqual' strict=0 n=500 type='Float32Array'         ***      6.94 %       ±2.34% ±3.15%  ±4.18%
assert/deepequal-typedarrays.js len=5000 method='deepEqual' strict=0 n=500 type='Float32Array'           ***    183.33 %       ±2.96% ±4.01%  ±5.35%
assert/deepequal-typedarrays.js len=5000 method='notDeepEqual' strict=0 n=500 type='Float32Array'        ***    208.51 %       ±7.26% ±9.91% ±13.43%

Refs: #50621

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Dec 4, 2023
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nodejs/v8 can you please have a look at this? It seems rather odd that the performance difference exists here.

@BridgeAR
Copy link
Member

BridgeAR commented Dec 4, 2023

Nit: @Septa2112 could you please update the commit message body's to include the "PR-URL"? That way the linter won't complain anymore 😃

@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@Septa2112
Copy link
Contributor Author

Septa2112 commented Dec 4, 2023

Nit: @Septa2112 could you please update the commit message body's "Refs" part to "PR-URL"? That way the linter won't complain anymore 😃

Thanks for your suggestion. But according to my understanding, the "PR-URL" will only be generated when my commit is merged into the main branch.

And I just referred to this PR #50621. Maybe the "Refs" should not be changed to "PR-URL"?

So is it better to delete this PR-URL line in my commit message? Or change the PR link to a related issue link? Just like Refs: https://github.com/nodejs/node/issues/50620.

@targos
Copy link
Member

targos commented Dec 4, 2023

Refs: is correct

@BridgeAR
Copy link
Member

BridgeAR commented Dec 4, 2023

Sorry, I meant to add that part. I misread the number and it can be added later on.

Improve performance of areSimilarFloatArrays by using primordial.

Refs: nodejs#50621
@anonrig anonrig added the needs-benchmark-ci PR that need a benchmark CI run. label Dec 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2023
@nodejs-github-bot nodejs-github-bot merged commit 1045f28 into nodejs:main Dec 10, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 1045f28

RafaelGSS pushed a commit that referenced this pull request Dec 15, 2023
Improve performance of areSimilarFloatArrays by using primordial.

Refs: #50621
PR-URL: #51040
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Dec 15, 2023
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Improve performance of areSimilarFloatArrays by using primordial.

Refs: #50621
PR-URL: #51040
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants