src: use SIMD for snapshot checksum

[kvakil]

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:

foriin{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]

Review requested:

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

[bnoordhuis]

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]

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_adler32Breakpoint 1 at 0x1f7aa20(gdb) rStarting 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&) ()...

[addaleax]

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

[kvakil]

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]

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.