deps: V8: use a 128-bit multiply in rapidhash secret generation - #65795
Closed
colinhacks wants to merge 1 commit into
Closed
deps: V8: use a 128-bit multiply in rapidhash secret generation#65795colinhacks wants to merge 1 commit into
colinhacks wants to merge 1 commit into
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
build
Contributor
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. |
Since the seeded array index hash landed, HashSeed::InitializeRoots runs rapidhash_make_secret on every isolate start, including every worker_threads Worker. The generator runs a 12-base Miller-Rabin test over ~1,500 rejection-sampled candidates per secret, and its mul_mod is a 64-iteration shift-add loop with two 64-bit modulos per iteration, ~9,000 calls per generation: 12-14% of the samples in a perf profile of `node -e 0` on Linux x64. Use a 128-bit multiply and modulo where the compiler runtime provides one and keep the loop as the fallback. Clang on Windows defines __SIZEOF_INT128__ but its runtime has no __umodti3 (compiler-rt builds the 128-bit division helpers for LP64 targets only), so Windows keeps the loop. The generated secrets are bit-identical: a standalone copy of the generator agrees on 6,000 words over 2,000 seeds on x64 and arm64 Linux, macOS and Windows, and runs 15-22x faster where the new path is taken. Measured on this tree, min of 100 runs, paired against an unpatched build on the same machine: `node -e 0` 19.75 -> 18.09 ms on Linux x64 and 30.8 -> 29.3 ms on macOS arm64, a Worker spawn 3.9 -> 3.1 ms. The `parallel`, `sequential`, `message` and `es-module` suites show no failure the unpatched build does not have. Refs: https://chromium-review.googlesource.com/c/v8/v8/+/6733490 Refs: nodejs@af5c144ebc Signed-off-by: Colin McDonnell <[email protected]>
colinhacks
force-pushed
the
v8-rapidhash-mulmod-int128
branch
from
September 4, 2026 15:18
d52830a to
44c689d
Compare
colinhacks
added a commit
to colinhacks/frizz
that referenced
this pull request
Sep 4, 2026
nodejs/node#65795 was opened 2026-09-04T15:12Z and frizz's first report 62s later read '✅ CI PASSED — 15 checks green'. The commit was never built. Two readings had to be wrong at once for that sentence to exist: - 12 of the 15 entries had concluded SKIPPED and the other 3 were label bots. githubWatchStatus folded SKIPPED into 'passed', so a rollup that asserted nothing read as a full green build. - the real 29-check matrix was held at GitHub's fork-approval gate. A gated workflow produces NO check run, so it is absent from statusCheckRollup entirely — the rollup looked complete because the missing half was invisible. Frizz was already fetching the answer: defaultFetchPr lists the head's workflow runs to name failed jobs, and all 8 gated workflows are in that list at conclusion ACTION_REQUIRED. failedCheckNames skipped them (a pending approval is not a failure, which is right) and then nothing else looked. - skipped is counted apart from passed, and 'passing' now needs a real SUCCESS - gated>0 reads as running, never as a verdict, and is reported in its own right: it is the one CI state that never resolves on its own, so silence about it is a dead wait - the green wake line carries the skip count; the gated line says a maintainer has to approve, so the worker asks instead of waiting - the chat divider drew any non-passing verdict as 'CI failed' — gated now reads 'CI awaiting approval', and the card shows 'N awaiting approval' with a static attention mark rather than a spinner promising motion that is not happening evalRollup is left alone and its header now says why: it reads the rollup and cannot see a gate. Anything wanting the true verdict goes through githubWatchStatus, which is given the workflow runs too.
Member
|
👋 This feels like it would be accepted upstream? (rapidhash already uses this approach for native int128 arithmetic: https://chromium.googlesource.com/v8/v8/+/refs/tags/15.5.11/third_party/rapidhash-v8/rapidhash.h#168) |
Author
|
Applies unchanged to V8 main, |
Member
|
Sure, what I mean is that we wouldn't ordinarily float patches that would be fine to submit upstream; we'd wait for them to be submitted and accepted first, then cherry-pick the resulting commit(s) from V8. |
Author
|
Gotcha, closing. Sorry for the noise. I'll look into upstreaming . 👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since the seeded array index hash landed (af5c144), every isolate start runs
rapidhash_make_secret, which searches for three random 64-bit primes with a 12-base Miller-Rabin test. The cost is inmul_mod, a 64-iteration shift-add loop with two 64-bit modulos per iteration, called about 9,000 times per generation. It is the top symbol in aperfprofile ofnode -e 0on Linux x64: 12-14% of samples on the v25 and v26 official binaries, absent on v24.This patch computes
mul_modwith a 128-bit multiply and modulo where the compiler runtime provides one and keeps the loop as the fallback. The secrets are bit-identical: a standalone copy of the generator produces the same words for 50,000 seeds with both forms on x64 and arm64 Linux and macOS, with gcc and clang, and for 3,000 seeds on s390x, ppc64le and riscv64 under QEMU. UBSan and ASan report nothing. Windows keeps the loop. Clang-cl defines__SIZEOF_INT128__, but its runtime has no__umodti3(compiler-rt builds the 128-bit division helpers for LP64 targets only), so the fast path is guarded with!defined(_WIN32).Measured against an unpatched build of the same tree, min of 100 runs, both binaries interleaved on one machine:
node -e 0, Linux x64 (n2-standard-64, idle)node -e 0, macOS arm64 (M1 Max)worker_threadsWorker spawn, macOS arm64v8Starttoenvironmentinperformance.nodeTiming, Linux x64The profile no longer shows
sprporHashSeed::InitializeRoots. Theparallel,sequential,messageandes-modulesuites on Linux x64 show no failure the unpatched build does not have (test-permission-drop-ffifails on both).This is a floating patch on Node's V8 copy, so
v8_embedder_stringmoves to-node.30and the change is listed inREADME.chromium. The diff applies unchanged to V8 main; the upstream tracking bug for the generator is crbug.com/409717082. The v24, v22 and v20 lines carry the same generator since the CVE-2026-21717 backport and pay the same cost.Refs: https://chromium-review.googlesource.com/c/v8/v8/+/6733490
Disclosure per the AI use policy: this change was prepared with an AI coding agent directed by the author, including the investigation, the patch, the measurements above and this description, and the agent opened the pull request through the GitHub CLI on the author's instruction. Verification: Node built at 5323423 on Linux x64 (gcc 13) and macOS 15 arm64; the generated secrets compared with a standalone copy of the generator on Linux, macOS and Windows toolchains and under QEMU for s390x, ppc64le, riscv64 and aarch64 (https://github.com/nubjs/nub/tree/probe/node-startup/tests/node-mulmod-probe); the
parallel,sequential,messageandes-modulesuites run against an unpatched build.