Uh oh!
There was an error while loading. Please reload this page.
build: add configure option --v8-enable-short-builtin-calls - #42109
Conversation
nodejs-github-bot
commented
Feb 24, 2022
Review requested:
|
targos
commented
Feb 24, 2022
Is this a breaking change for native addons? |
qdaoming
commented
Feb 24, 2022
No, this is not a breaking change. |
qdaoming
commented
Feb 24, 2022
For the failed check "First commit message adheres to guidelines at https://goo.gl/p2fr5Q /...", I have modified the title format of this commit, but don't know how to rerun this failed check. |
targos
commented
Feb 24, 2022
Should we be able to measure the performance improvement with our micro benchmarks? |
qdaoming
commented
Feb 24, 2022
We have tested the performance with some internal node.js application benchmarks, and short-builtin-calls(together with sparkplug) can bring +6% performance increase. |
targos
commented
Feb 26, 2022
@nodejs/build @nodejs/v8 |
nodejs-github-bot
commented
Feb 26, 2022
Benchmark run on the Results |
qdaoming
commented
Mar 1, 2022
I have modified the commit message to fix the lint error and now all checks have passed. |
nodejs-github-bot
commented
Mar 7, 2022
For CI tests, this commit passed all of them about 1 weeks ago. |
nodejs-github-bot
commented
Mar 18, 2022
nodejs-github-bot
commented
Mar 18, 2022
This comment was marked as outdated.
This comment was marked as outdated.
nodejs-github-bot
commented
Apr 11, 2022
Trott
commented
Apr 11, 2022
Is it possible that the consistent failures on fedora-latest-x64 in CI are due to the change in this PR? I'm not seeing it anywhere else, and it's consistent here.... |
gengjiawen
commented
Apr 12, 2022
Thx for the info. I was using retry button. Looks I click the wrong button.
Old build works fine, not sure why. https://ci.nodejs.org/job/node-test-pull-request/43380/ Looks like something wrong with Jenkins cc @richardlau @nodejs/build |
nodejs-github-bot
commented
Apr 12, 2022
nodejs-github-bot
commented
Apr 12, 2022
I rebooted test-digitalocean-fedora34-x64-1 and the CI now passes on it 🤷. |
nodejs-github-bot
commented
Apr 12, 2022
Landed in aa52873 |
gengjiawen
commented
Apr 12, 2022
@qdaoming-intel Thx for the contribution. (sorry the commit-queue mess and take so long to merge) |
qdaoming
commented
Apr 13, 2022
@gengjiawen Thank you and all other people's support. |
Add configure option --v8-enable-short-builtin-calls and enable it by default on x86_64 platform. PR-URL: nodejs#42109 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
juanarbol
commented
May 30, 2022
This commit breaks build/compiling C++ in v16.x, is says something like: |
qdaoming
commented
Jun 2, 2022
Please don't merge this commit to v16.x, because this commit depends on some new feature in V8, and v16.x doesn't have such feature. |
Add configure option --v8-enable-short-builtin-calls
and enable it by default on x86_64 platform.