Skip to content

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 8, 2025

Should provide better performance on some platforms.

                       confidence improvement accuracy (*)   (**)  (***)
util/trig.js n=1000000        ***     37.41 %       ±2.11% ±2.81% ±3.67%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 1 comparisons, you can thus
expect the following amount of false-positive results:
  0.05 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.01 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

Context: @t3dotgg put together some benchmarks that show the trig functions running in node.js aren't as fast as they could be. We did some digging and think this may help. https://github.com/t3dotgg/cf-vs-vercel-bench/blob/main/vanilla-bench/vercel-edition/api/

There's a possibility this won't work on all platforms/archs so running some tests in CI to verify first. Appears to be fine in CI!

/cc @nodejs/v8

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Oct 8, 2025
@jasnell jasnell marked this pull request as ready for review October 8, 2025 00:29
@nodejs-github-bot

This comment was marked as outdated.

@jasnell jasnell added v8 engine Issues and PRs related to the V8 dependency. performance Issues and PRs related to the performance of Node.js. and removed performance Issues and PRs related to the performance of Node.js. labels Oct 8, 2025
Should provide better performance on some platforms.
@nodejs-github-bot

This comment was marked as outdated.

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 8, 2025

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 8, 2025
@devsnek
Copy link
Member

devsnek commented Oct 8, 2025

just a note of interest... several years ago I tried applying libm optimizations to some operations in V8 (though, not the trig functions) and we eventually found the output to be bad enough in certain cases to necessitate reverting it.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

I don't oppose to the idea but I don't see how this change can have any effect, given that the build flag is not implemented in the GYP files:

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants