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: speedup compilation of mksnapshot output #48162

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

kvakil
Copy link
Contributor

@kvakil kvakil commented May 25, 2023

Incremental compilation of Node.js is slow. Currently on a powerful
Linux machine, it takes about 5.8 seconds to compile
gen/node_snapshot.cc with g++.

As in the previous PR which dealt with node_js2c, we add a new build
define NODE_MKSNAPSHOT_USE_STRING_LITERALS which is used by
node_mksnapshot. When this flag is set, we emit string literals
instead of array literals for the snapshot blob and for the code cache,
i.e.:

// old: static const uint8_t X[] = { ... };
static const uint8_t *X = "...";

I only enabled the new flag on Linux/macOS, since those are systems that
I have available for testing. On my Linux system with gcc, it speeds up
compilation of this file by 3.7s (5.8s -> 2.1s). On my Mac system with
clang, it speeds up compilation by 1.7s (3.4s -> 1.7s).

Again, the right thing here is probably to generate separate files for
the snapshot blob and for each code cache output, but this is a nice
intermediate speedup.

Refs: #47984
Refs: #48160

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 25, 2023
src/node_snapshotable.cc Outdated Show resolved Hide resolved
src/node_snapshotable.cc Outdated Show resolved Hide resolved
tools/snapshot/node_mksnapshot.cc Outdated Show resolved Hide resolved
@bnoordhuis
Copy link
Member

If we're doing toolchain-specific speed hacks: a trick I've used in other projects is to emit the .o directly. Much (much!) faster than first generating, then parsing a huge C source file.

I only had to care about x86_64 Linux though. For node it'd be a bit more complex.

@aduh95
Copy link
Contributor

aduh95 commented May 25, 2023

This makes the test-linux GHA ends 30 minutes earlier than on other PRs, which is impressive and seems to be worth it by itself even if it had no effect on incremental compilation!

Incremental compilation of Node.js is slow. Currently on a powerful
Linux machine, it takes about 5.8 seconds to compile
`gen/node_snapshot.cc` with g++.

As in the previous PR which dealt with `node_js2c`, we add a new build
define `NODE_MKSNAPSHOT_USE_STRING_LITERALS` which is used by
`node_mksnapshot`. When this flag is set, we emit string literals
instead of array literals for the snapshot blob and for the code cache,
i.e.:

```c++
// old: static const uint8_t X[] = { ... };
static const uint8_t *X = "...";
```

I only enabled the new flag on Linux/macOS, since those are systems that
I have available for testing. On my Linux system with gcc, it speeds up
compilation of this file by 3.7s (5.8s -> 2.1s). On my Mac system with
clang, it speeds up compilation by 1.7s (3.4s -> 1.7s).

Again, the right thing here is probably to generate separate files for
the snapshot blob and for each code cache output, but this is a nice
intermediate speedup.

Refs: nodejs#47984
Refs: nodejs#48160
@kvakil
Copy link
Contributor Author

kvakil commented May 25, 2023

If we're doing toolchain-specific speed hacks: a trick I've used in other projects is to emit the .o directly. Much (much!) faster than first generating, then parsing a huge C source file.

I only had to care about x86_64 Linux though. For node it'd be a bit more complex.

Wow, generating a .o with objcopy is really fast. There's more good suggestions in this blog, apparently using .incbin with an assembler is about equally fast as objcopy/ld.

I guess, the advantage of this approach is that it's simpler (no extra files). I may try to put up another diff using incbin or similar.


This makes the test-linux GHA ends 30 minutes earlier than on other PRs, which is impressive and seems to be worth it by itself even if it had no effect on incremental compilation!

That's got to be some sort of fluke, I don't expect it to help that much.


Thanks Joyee for the suggestion to switch to a build flag, which makes this diff much shorter. (I'm going to apply a similar treatment to #48160.)

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

@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2023
@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonrig
Copy link
Member

anonrig commented May 31, 2023

@kvakil the build is failing

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen
Copy link
Contributor

It's probably a flake because the error was about missing .tap files. I have started a fresh build.

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 1, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 1, 2023
@nodejs-github-bot nodejs-github-bot merged commit 287dada into nodejs:main Jun 1, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 287dada

targos pushed a commit that referenced this pull request Jun 4, 2023
Incremental compilation of Node.js is slow. Currently on a powerful
Linux machine, it takes about 5.8 seconds to compile
`gen/node_snapshot.cc` with g++.

As in the previous PR which dealt with `node_js2c`, we add a new build
define `NODE_MKSNAPSHOT_USE_STRING_LITERALS` which is used by
`node_mksnapshot`. When this flag is set, we emit string literals
instead of array literals for the snapshot blob and for the code cache,
i.e.:

```c++
// old: static const uint8_t X[] = { ... };
static const uint8_t *X = "...";
```

I only enabled the new flag on Linux/macOS, since those are systems that
I have available for testing. On my Linux system with gcc, it speeds up
compilation of this file by 3.7s (5.8s -> 2.1s). On my Mac system with
clang, it speeds up compilation by 1.7s (3.4s -> 1.7s).

Again, the right thing here is probably to generate separate files for
the snapshot blob and for each code cache output, but this is a nice
intermediate speedup.

Refs: #47984
Refs: #48160
PR-URL: #48162
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos targos mentioned this pull request Jun 4, 2023
franciszek-koltuniuk-red pushed a commit to franciszek-koltuniuk-red/node that referenced this pull request Jun 7, 2023
Incremental compilation of Node.js is slow. Currently on a powerful
Linux machine, it takes about 5.8 seconds to compile
`gen/node_snapshot.cc` with g++.

As in the previous PR which dealt with `node_js2c`, we add a new build
define `NODE_MKSNAPSHOT_USE_STRING_LITERALS` which is used by
`node_mksnapshot`. When this flag is set, we emit string literals
instead of array literals for the snapshot blob and for the code cache,
i.e.:

```c++
// old: static const uint8_t X[] = { ... };
static const uint8_t *X = "...";
```

I only enabled the new flag on Linux/macOS, since those are systems that
I have available for testing. On my Linux system with gcc, it speeds up
compilation of this file by 3.7s (5.8s -> 2.1s). On my Mac system with
clang, it speeds up compilation by 1.7s (3.4s -> 1.7s).

Again, the right thing here is probably to generate separate files for
the snapshot blob and for each code cache output, but this is a nice
intermediate speedup.

Refs: nodejs#47984
Refs: nodejs#48160
PR-URL: nodejs#48162
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jul 4, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Incremental compilation of Node.js is slow. Currently on a powerful
Linux machine, it takes about 5.8 seconds to compile
`gen/node_snapshot.cc` with g++.

As in the previous PR which dealt with `node_js2c`, we add a new build
define `NODE_MKSNAPSHOT_USE_STRING_LITERALS` which is used by
`node_mksnapshot`. When this flag is set, we emit string literals
instead of array literals for the snapshot blob and for the code cache,
i.e.:

```c++
// old: static const uint8_t X[] = { ... };
static const uint8_t *X = "...";
```

I only enabled the new flag on Linux/macOS, since those are systems that
I have available for testing. On my Linux system with gcc, it speeds up
compilation of this file by 3.7s (5.8s -> 2.1s). On my Mac system with
clang, it speeds up compilation by 1.7s (3.4s -> 1.7s).

Again, the right thing here is probably to generate separate files for
the snapshot blob and for each code cache output, but this is a nice
intermediate speedup.

Refs: nodejs#47984
Refs: nodejs#48160
PR-URL: nodejs#48162
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Incremental compilation of Node.js is slow. Currently on a powerful
Linux machine, it takes about 5.8 seconds to compile
`gen/node_snapshot.cc` with g++.

As in the previous PR which dealt with `node_js2c`, we add a new build
define `NODE_MKSNAPSHOT_USE_STRING_LITERALS` which is used by
`node_mksnapshot`. When this flag is set, we emit string literals
instead of array literals for the snapshot blob and for the code cache,
i.e.:

```c++
// old: static const uint8_t X[] = { ... };
static const uint8_t *X = "...";
```

I only enabled the new flag on Linux/macOS, since those are systems that
I have available for testing. On my Linux system with gcc, it speeds up
compilation of this file by 3.7s (5.8s -> 2.1s). On my Mac system with
clang, it speeds up compilation by 1.7s (3.4s -> 1.7s).

Again, the right thing here is probably to generate separate files for
the snapshot blob and for each code cache output, but this is a nice
intermediate speedup.

Refs: nodejs#47984
Refs: nodejs#48160
PR-URL: nodejs#48162
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants