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

src: use SIMD for snapshot checksum #47145

Closed
wants to merge 1 commit into from
Closed

Conversation

kvakil
Copy link
Contributor

@kvakil kvakil commented Mar 18, 2023

Snapshot checksum verification uses v8::Internal::Checksum, which uses zlib's adler32 checksum. However it was using the slow version of adler32 for machines with no SIMD. Let's change it to use the SIMD version when it is available. Fortunately the necessary incantations were already included in our other copy of Chromium's zlib in deps/zlib/zlib.gyp, so this is mostly just copying from there.

On a very recent Intel machine, this speeds up the snapshot checksumming from 870us to 300us (a 570us startup saving), according to:

for i in {1..100}; do
  ./node --profile-deserialization -e 0
done \
  |& grep 'Verifying snapshot checksum' \
  | awk '{s+=$5} END{print s/NR}'

P.S.: maybe we should just disable this entirely via --no-verify-snapshot-checksum? It doesn't feel like a super necessary protection.

Snapshot checksum verification uses `v8::Internal::Checksum`, which uses
zlib's adler32 checksum. However it was using the slow version of
adler32 for machines with no SIMD. Let's change it to use the SIMD
version when it is available. Fortunately the necessary incantations
were already included in our _other_ copy of Chromium's zlib in
`deps/zlib/zlib.gyp`, so this is mostly just copying from there.

On a very recent Intel machine, this speeds up the snapshot checksumming
from 870us to 300us (a 570us startup saving), according to:

```bash
for i in {1..100}; do
  ./node --profile-deserialization -e 0
done \
  |& grep 'Verifying snapshot checksum' \
  | awk '{s+=$5} END{print s/NR}'
```

P.S.: maybe we should just disable this entirely via
`--no-verify-snapshot-checksum`? It doesn't feel like a super necessary
protection.
@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 18, 2023
@kvakil kvakil added c++ Issues and PRs that require attention from people who are familiar with C++. snapshot Issues and PRs related to the startup snapshot labels Mar 18, 2023
@bnoordhuis
Copy link
Member

Perhaps you can just use deps/zlib and get rid of v8's fork? Code duplication is bad enough, I'd rather not duplicate the magic build incantations as well. :-)

P.S.: maybe we should just disable this entirely via --no-verify-snapshot-checksum? It doesn't feel like a super necessary protection.

It's off by default in release builds. If you're seeing a performance improvement, it's either because you're testing with a debug build or because of something else (snapshot compression? but that was disabled recently in #45716.)

@kvakil
Copy link
Contributor Author

kvakil commented Mar 18, 2023

Perhaps you can just use deps/zlib and get rid of v8's fork? Code duplication is bad enough, I'd rather not duplicate the magic build incantations as well. :-)

I'll give it a go and report back. It's a little weird because v8 expects these the symbols to be defined with the prefix Cr_z_ (so like Cr_z_adler32) in deps/v8/third_party/zlib/chromeconf.h, so we'll need to float a patch to disable that. (& of course this may require that we update zlib at the same time as updating V8, but that requirement doesn't feel too onerous.)

P.S.: maybe we should just disable this entirely via --no-verify-snapshot-checksum? It doesn't feel like a super necessary protection.

It's off by default in release builds. If you're seeing a performance improvement, it's either because you're testing with a debug build or because of something else (snapshot compression? but that was disabled recently in #45716.)

Yeah, I was wrong: it turns out the flag does not effect it either way. It is controlled by the generated output in mksnapshot.cc. For example this seems to make it go away:

diff --git a/deps/v8/src/snapshot/mksnapshot.cc b/deps/v8/src/snapshot/mksnapshot.cc
index dd67969694..23fb00aab9 100644
--- a/deps/v8/src/snapshot/mksnapshot.cc
+++ b/deps/v8/src/snapshot/mksnapshot.cc
@@ -87,7 +87,7 @@ class SnapshotFileWriter {
     fprintf(
         fp,
         "bool Snapshot::ShouldVerifyChecksum(const v8::StartupData* data) {\n");
-    fprintf(fp, "  return true;\n");
+    fprintf(fp, "  return false;\n");
     fprintf(fp, "}\n");
     fprintf(fp, "}  // namespace internal\n");
     fprintf(fp, "}  // namespace v8\n");

But other than that, this is happening on release builds AFAICT:

$ gdb out/Release/node
(gdb) b Cr_z_adler32
Breakpoint 1 at 0x1f7aa20
(gdb) r
Starting program: /home/ubuntu/node/out/Release/node 
Thread 1 "node" hit Breakpoint 1, 0x000055555737aa20 in Cr_z_adler32 ()
(gdb) bt
#0  0x000055555737aa20 in Cr_z_adler32 ()
#1  0x000055555674e3dc in v8::internal::Checksum(v8::base::Vector<unsigned char const>) ()
#2  0x000055555674e6d5 in v8::internal::Snapshot::Initialize(v8::internal::Isolate*) [clone .part.0] ()
#3  0x000055555608954b in v8::Isolate::Initialize(v8::Isolate*, v8::Isolate::CreateParams const&) ()
...

kvakil added a commit to kvakil/node that referenced this pull request Mar 19, 2023
We currently have two copies of Chromium's zlib: one in deps/zlib and
another in deps/v8/third_party/zlib. This has a couple of disadvantages:

1. There is an additional cost to keeping both dependencies up-to-date,
    and in fact they were already out-of-sync (see the refs).
2. People who compile with --shared-zlib (i.e. distro maintainers) will
   probably not be thrilled to learn that there is still a copy of zlib
   inside.
3. It's aesthetically unpleasing.

Centralize on the version in V8 rather than the one in deps, and delete
the one in deps. Basically I just copied deps/zlib/zlib.gyp to
tools/v8_gypfiles/zlib.gyp, since the former had a better build
configuration (see the refs). This approach seemed better than the
opposite approach of centralizing on deps/zlib because:

1. We would need to occasionally bump deps/zlib manually after bumping
   deps/v8, which seemed annoying.
2. The maintenance steps for bumping zlib seemed more onerous than the
   maintenance steps for bumping V8.

(If we would prefer the opposite approach, I actually have another patch
 locally.)

One discrepancy was that V8's version of zlib had all symbols to be
prefixed with `Cr_z_` per deps/v8/third_party/zlib/chromeconf.h, which
seemed undesirable, so I added define `CHROMIUM_ZLIB_NO_CHROMECONF`
to the build to remove it. (deps/zlib handled this by just commenting
out the relevant include, but floating a patch seemed less desirable.)

I tested this on Linux with the default build and a --shared-zlib build.
I checked that the shared-zlib build correctly linked zlib according to
ldd. I would appreciate if the reviewers could suggest some other build
configurations to try.

Refs: nodejs#47145
Refs: nodejs#47151
@kvakil kvakil mentioned this pull request Mar 19, 2023
},
}],
],
}],
Copy link
Member

Choose a reason for hiding this comment

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

It's pretty unclear why it would be safe to assume SSSE3 support here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is copied from deps/zlib/zlib.gyp; runtime CPU detection is used to decide which SIMD version to call into. (Or are you suggesting that we should add a comment to that effect?)

kvakil added a commit to kvakil/node that referenced this pull request Apr 10, 2023
We currently have two copies of Chromium's zlib: one in deps/zlib and
another in deps/v8/third_party/zlib. This has a couple of disadvantages:

1. There is an additional cost to keeping both dependencies up-to-date,
   and in fact they were already out-of-sync (see the refs).

2. People who compile with --shared-zlib (i.e. distro maintainers) will
   probably not be thrilled to learn that there is still a copy of zlib
   inside.

3. It's aesthetically unpleasing.

This diff (per discussion in the refs) centralizes on deps/zlib and
deletes deps/v8/third_party/zlib. That requires changing the include
headers in two files in deps/v8/src, which was pretty straightforward.

When the user requests compiling with a shared zlib build, we still need
to compile the contents of deps/zlib/google. This is not ideal but it's
also not a change in behavior: prior to this diff those files were
being compiled in the deps/v8/third_party/zlib version.

I tested this on Linux with the default build and a --shared-zlib build.
I checked that the shared-zlib build dynamically linked zlib according
to ldd, and that the regular build did not. I would appreciate if the
reviewers could suggest some other build configurations to try.

Refs: nodejs#47145
Refs: nodejs#47157
kvakil added a commit to kvakil/node that referenced this pull request Apr 10, 2023
We currently have two copies of Chromium's zlib: one in deps/zlib and
another in deps/v8/third_party/zlib. This has a couple of disadvantages:

1. There is an additional cost to keeping both dependencies up-to-date,
   and in fact they were already out-of-sync (see the refs).

2. People who compile with --shared-zlib (i.e. distro maintainers) will
   probably not be thrilled to learn that there is still a copy of zlib
   inside.

3. It's aesthetically unpleasing.

This diff (per discussion in the refs) centralizes on deps/zlib and
deletes deps/v8/third_party/zlib. That requires changing the include
headers in two files in deps/v8/src, which was pretty straightforward.

When the user requests compiling with a shared zlib build, we still need
to compile the contents of deps/zlib/google. This is not ideal but it's
also not a change in behavior: prior to this diff those files were
being compiled in the deps/v8/third_party/zlib version.

I tested this on Linux with the default build and a --shared-zlib build.
I checked that the shared-zlib build dynamically linked zlib according
to ldd, and that the regular build did not. I would appreciate if the
reviewers could suggest some other build configurations to try.

Refs: nodejs#47145
Refs: nodejs#47157
kvakil added a commit to kvakil/node that referenced this pull request Apr 10, 2023
We currently have two copies of Chromium's zlib: one in deps/zlib and
another in deps/v8/third_party/zlib. This has a couple of disadvantages:

1. There is an additional cost to keeping both dependencies up-to-date,
   and in fact they were already out-of-sync (see the refs).

2. People who compile with --shared-zlib (i.e. distro maintainers) will
   probably not be thrilled to learn that there is still a copy of zlib
   inside.

3. It's aesthetically unpleasing.

This diff (per discussion in the refs) centralizes on deps/zlib and
deletes deps/v8/third_party/zlib. That requires changing the include
headers in two files in deps/v8/src, which was pretty straightforward.

When the user requests compiling with a shared zlib build, we still need
to compile the contents of deps/zlib/google. This is not ideal but it's
also not a change in behavior: prior to this diff those files were
being compiled in the deps/v8/third_party/zlib version.

I tested this on Linux with the default build and a --shared-zlib build.
I checked that the shared-zlib build dynamically linked zlib according
to ldd, and that the regular build did not. I would appreciate if the
reviewers could suggest some other build configurations to try.

Refs: nodejs#47145
Refs: nodejs#47157
kvakil added a commit to kvakil/node that referenced this pull request Jun 22, 2023
We currently have two copies of Chromium's zlib: one in deps/zlib and
another in deps/v8/third_party/zlib. This has a couple of disadvantages:

1. There is an additional cost to keeping both dependencies up-to-date,
   and in fact they were already out-of-sync (see the refs).

2. People who compile with --shared-zlib (i.e. distro maintainers) will
   probably not be thrilled to learn that there is still a copy of zlib
   inside.

3. It's aesthetically unpleasing.

This diff (per discussion in the refs) centralizes on deps/zlib and
deletes deps/v8/third_party/zlib. That requires changing the include
headers in two files in deps/v8/src, which was pretty straightforward.

When the user requests compiling with a shared zlib build, we still need
to compile the contents of deps/zlib/google. This is not ideal but it's
also not a change in behavior: prior to this diff those files were
being compiled in the deps/v8/third_party/zlib version.

I tested this on Linux with the default build and a --shared-zlib build.
I checked that the shared-zlib build dynamically linked zlib according
to ldd, and that the regular build did not. I would appreciate if the
reviewers could suggest some other build configurations to try.

Refs: nodejs#47145
Refs: nodejs#47157
kvakil added a commit to kvakil/node that referenced this pull request Jun 22, 2023
This is the first of a series of patches. This patch is contains changes
to the existing zlib.gyp file to allow it to be used by our v8.gyp.

---

We currently have two copies of Chromium's zlib: one in deps/zlib and
another in deps/v8/third_party/zlib. This has a couple of disadvantages:

1. There is an additional cost to keeping both dependencies up-to-date,
   and in fact they were already out-of-sync (see the refs).

2. People who compile with --shared-zlib (i.e. distro maintainers) will
   probably not be thrilled to learn that there is still a copy of zlib
   inside.

3. It's aesthetically unpleasing.

This diff (per discussion in the refs) centralizes on deps/zlib and
deletes deps/v8/third_party/zlib.

When the user requests compiling with a shared zlib build, we still need
to compile the contents of deps/zlib/google. This is not ideal but it's
also not a change in behavior: prior to this diff those files were
being compiled in the deps/v8/third_party/zlib version.

I tested this on Linux with the default build and a --shared-zlib build.
I checked that the shared-zlib build dynamically linked zlib according
to ldd, and that the regular build did not. I would appreciate if the
reviewers could suggest some other build configurations to try.

Refs: nodejs#47145
Refs: nodejs#47157
@kvakil
Copy link
Contributor Author

kvakil commented Jun 22, 2023

Closing for #47493. In addition this issue is sort of already fixed on the V8 side by https://chromium-review.googlesource.com/c/v8/v8/+/4571989.

@kvakil kvakil closed this Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. snapshot Issues and PRs related to the startup snapshot 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.

4 participants