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

build: speed up compilation of some V8 files #52083

Closed
wants to merge 3 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Mar 14, 2024

This introduces a special target to compile some of the
'v8_initializers' files with "-O1" instead of "-O3" to avoid huge
compilation times with GCC versions <13.

Closes: #52068

This introduces a special target to compile some of the
'v8_initializers' files with "-O1" instead of "-O3" to avoid huge
compilation times with GCC versions <13.

Closes: nodejs#52068
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Mar 14, 2024
@targos targos added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Mar 14, 2024
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lemire
Copy link
Member

lemire commented Mar 14, 2024

@targos You went with -O1, presumably because -O2 won't cut it?

@targos
Copy link
Member Author

targos commented Mar 14, 2024

@lemire Yes. I tested -O3, -O2 and -O1 with GCC 12.2.1 on Rocky Linux 8.

@nodejs-github-bot
Copy link
Collaborator

@lemire
Copy link
Member

lemire commented Mar 14, 2024

@targos In the diff, I don't see GCC version detection... it says that the issue is fixed with GCC 13. Is there the equivalent of "if GCC >= 13 do this, else do that", or is that something that must be done manually later?

@targos
Copy link
Member Author

targos commented Mar 14, 2024

@lemire I don't know, and I'm not interested in looking into it.

@lemire
Copy link
Member

lemire commented Mar 14, 2024

I don't know, and I'm not interested in looking into it.

Here is my proposal: we merge this PR but we add an issue where we saw that this workaround should be revisited once GCC >= 13 can be safely assumed.

My concern here is that we might be trading off performance, but in a way that is non-obvious.

@nodejs-github-bot
Copy link
Collaborator

@targos targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 14, 2024
@joyeecheung
Copy link
Member

If -O2 doesn't make it faster, does -Os make a difference?

It would be useful to measure if this affects wasm <-> JS interop performance. For example, a benchmark involving import 'cjs.cjs' where cjs.cjs contains a bunch of exports (because the cjs-module-lexer hits this path).

@joyeecheung
Copy link
Member

Actually I think there already is a benchmark for wasm/js interop via the cjs exports detction. Benchmark CI https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1496/

@targos
Copy link
Member Author

targos commented Mar 16, 2024

  • -O2: ~1h
  • -Os: 2m56s
  • -O1: 18s

@bnoordhuis
Copy link
Member

For posterity:

12:36:48                        confidence improvement accuracy (*)   (**)  (***)
12:36:48 esm/cjs-parse.js n=100                -1.60 %       ±5.70% ±7.59% ±9.89%

Vs. a 200x speedup in compile times? Done, -O1 it is.

@joyeecheung
Copy link
Member

How much difference does -Os make in the binary size? I am not able to find where the object files are locally, the two generated .cc files appear to be around 400KB. While that’s not too big as far as generated file goes, not sure if the problematic path in GCC can lead to any surprising results here. I’d say if the binary size differences is >5m, then -Os would be better. (2m doesn’t seem that bad for V8). Otherwise -O1 is fine.

Copy link
Member

@lemire lemire left a comment

Choose a reason for hiding this comment

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

I think that this is fine as-is.

@targos
Copy link
Member Author

targos commented Mar 18, 2024

With -O1:

$ ls -alsh out/Release/obj/tools/v8_gypfiles/gen/torque-generated/src/builtins/v8_initializers_slow.js-to-wasm-tq-csa.o out/Release/obj/tools/v8_gypfiles/gen/torque-generated/src/builtins/v8_initializers_slow.wasm-to-js-tq-csa.o out/Release/obj/tools/v8_gypfiles/libv8_initializers_slow.a out/Release/node
112M -rwxrwxr-x 1 mzasso mzasso 103M Mar 18 09:20 out/Release/node
660K -rw-rw-r-- 1 mzasso mzasso 659K Mar 18 09:20 out/Release/obj/tools/v8_gypfiles/gen/torque-generated/src/builtins/v8_initializers_slow.js-to-wasm-tq-csa.o
496K -rw-rw-r-- 1 mzasso mzasso 496K Mar 18 09:20 out/Release/obj/tools/v8_gypfiles/gen/torque-generated/src/builtins/v8_initializers_slow.wasm-to-js-tq-csa.o
 40K -rw-rw-r-- 1 mzasso mzasso  38K Mar 18 09:20 out/Release/obj/tools/v8_gypfiles/libv8_initializers_slow.a

With -Os (note that I have to edit the .ninja file manually to test it, it's somehow ignored when set in the cflags):

$ ls -alsh out/Release/obj/tools/v8_gypfiles/gen/torque-generated/src/builtins/v8_initializers_slow.js-to-wasm-tq-csa.o out/Release/obj/tools/v8_gypfiles/gen/torque-generated/src/builtins/v8_initializers_slow.wasm-to-js-tq-csa.o out/Release/obj/tools/v8_gypfiles/libv8_initializers_slow.a out/Release/node
112M -rwxrwxr-x 1 mzasso mzasso 103M Mar 18 09:30 out/Release/node
544K -rw-rw-r-- 1 mzasso mzasso 544K Mar 18 09:26 out/Release/obj/tools/v8_gypfiles/gen/torque-generated/src/builtins/v8_initializers_slow.js-to-wasm-tq-csa.o
412K -rw-rw-r-- 1 mzasso mzasso 410K Mar 18 09:30 out/Release/obj/tools/v8_gypfiles/gen/torque-generated/src/builtins/v8_initializers_slow.wasm-to-js-tq-csa.o
 60K -rw-rw-r-- 1 mzasso mzasso  60K Mar 18 09:30 out/Release/obj/tools/v8_gypfiles/libv8_initializers_slow.a

targos added a commit that referenced this pull request Mar 19, 2024
This introduces a special target to compile some of the
'v8_initializers' files with "-O1" instead of "-O3" to avoid huge
compilation times with GCC versions <13.

PR-URL: #52083
Fixes: #52068
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
@targos
Copy link
Member Author

targos commented Mar 19, 2024

Landed in 4e278f0

@targos targos closed this Mar 19, 2024
@targos targos deleted the fix-slow-tq-csa branch March 19, 2024 07:50
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
This introduces a special target to compile some of the
'v8_initializers' files with "-O1" instead of "-O3" to avoid huge
compilation times with GCC versions <13.

PR-URL: nodejs#52083
Fixes: nodejs#52068
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
@marco-ippolito marco-ippolito added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label May 2, 2024
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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build: wasm-to-js-tq-csa.cc takes ages to compile
9 participants