GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — util: improve performance of function areSimilarFloatArrays by Septa2112 · Pull Request #51040 · nodejs/node

Conversation

@Septa2112

@Septa2112 Septa2112 commented Dec 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
meyfa, bricss, and H4ad reacted with rocket emoji
All reactions
  • 🚀 3 reactions
@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
BridgeAR
BridgeAR approved these changes Dec 4, 2023
View reviewed changes

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@BridgeAR

BridgeAR commented Dec 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 4, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 4, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 4, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56068/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Septa2112

Septa2112 commented Dec 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.
BridgeAR reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

targos commented Dec 4, 2023

Copy link
Copy Markdown
Member

Refs: is correct
BridgeAR reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Dec 4, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Septa2112
util: improve performance of function areSimilarFloatArrays
0e48964 
Improve performance of areSimilarFloatArrays by using primordial.

Refs: nodejs#50621
H4ad
H4ad approved these changes Dec 4, 2023
View reviewed changes
@anonrig anonrig added the needs-benchmark-ci PRs that need a benchmark CI run. label Dec 4, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 6, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56119/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Dec 9, 2023
View reviewed changes
@lpinca lpinca added the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 10, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 10, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 1045f28 into nodejs:main Dec 10, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 10, 2023

Copy link
Copy Markdown
Collaborator

Landed in 1045f28
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RafaelGSS pushed a commit that referenced this pull request Dec 15, 2023
@Septa2112 @RafaelGSS
util: improve performance of function areSimilarFloatArrays
cd891b3 
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
v21.5.0 proposal #51166
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@Septa2112 @richardlau
util: improve performance of function areSimilarFloatArrays
c3b89c3 
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
v20.12.0 proposal #52212
Merged
@alejandrodev-vn alejandrodev-vn mentioned this pull request Nov 9, 2026
assert: avoid excess float array comparisons #67357
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@BridgeAR BridgeAR BridgeAR approved these changes
@H4ad H4ad H4ad approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs-benchmark-ci PRs 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@Septa2112 @BridgeAR @nodejs-github-bot @targos @jasnell @H4ad @lpinca @anonrig
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.