Repository navigation
Conversation
Original commit message:
[heap] Treat zero allocation throughput as a low allocation rate
Since https://crrev.com/c/5935313 allocation throughput is tracked with
SmoothedBytesAndDuration, which has no lower bound. A sampled throughput
of exactly 0 -- nothing was allocated, or the smoothed value decayed to
0 after a long idle period -- made ComputeMutatorUtilizationImpl()
return a mutator utilization of 0 ("maximal allocation"). For an isolate
that never allocates on the CppHeap, HasLowAllocationRate() is then
never true and the MemoryReducer cannot start a GC through its
low-allocation condition; only the 100s watchdog can. Node.js hits this
for any process that does not use node:vm.
Remove the special case. A mutator speed of 0 now yields a mutator
utilization of 1, i.e. the lowest possible allocation rate. gc_speed is
either the conservative default or at least 1 byte/ms, so the formula
stays well defined.
Bug: 570738027
Change-Id: I84cd51607df3c31c2f1e02bb2fe258e5506131ce
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8521518
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#110599}
Refs: v8/v8@2444c10
Fixes: nodejs#66564
Signed-off-by: Austin Burdine <austin@ghost.org>
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. |
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
2 participants
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.
Draft: v24.x backport of v8/v8@2444c10 (https://chromium-review.googlesource.com/c/v8/v8/+/8521518, crbug https://issues.chromium.org/issues/570738027). Kept as a draft until the v26.x backport (#66736) has had its time in a Current release, per #66692.
Fixes: #66564
Since V8's allocation throughput moved to
SmoothedBytesAndDuration(V8 13.1), a throughput of exactly 0 madeComputeMutatorUtilizationImpl()report "maximal allocation". Node never allocates on the CppHeap unlessnode:vmis used, soHeap::HasLowAllocationRate()was never true and the MemoryReducer could only start a GC through its 100 s watchdog. v24.x (V8 13.6) is affected; v22.x (V8 12.4) is not. Upstream removed the zero special case, so 0 now counts as the lowest allocation rate.Changes from the upstream commit (same as the v26.x backport):
heap.cc,heap.h,gc-tracer.h: applied unchanged.deps/v8/AUTHORS: same line, merged by hand.test/unittests/heap/gc-tracer-unittest.cc:GCTracer::SampleAllocation()takes 4 counters on V8 13.6, so the two new tests drop the external-memory argument.v8_embedder_stringbumped to-node.58.Verification:
v24.22.1-pre, V813.6.233.17-node.58);make -j8 test-only: cctest 242/242, JS 5744/5749. The 5 JS failures are environmental: twofs-cpsocket tests hit the macOS socket-path length limit (they pass with a shortNODE_TEST_DIR), andtest-worker-track-unmanaged-fds,test-worker-debugandtest-esm-import-meta-main-evalfail identically with theheap.ccchange reverted (unmanaged-fd warnings on this macOS version).make test's doc lint step also aborts in libuv (uv__io_poll) on this machine, independent of this change.v8_enable_static_roots = falseto match Node's gyp config):unittests/GCTracerTest.*andunittests/MemoryReducer.*25/25 pass. With theheap.ccchange reverted,GCTracerTest.LowAllocationRateWithZeroEmbedderAllocationfails (HasLowAllocationRate()is false); with it, both new tests pass.make test-v8suites: 14,141/14,149 pass. All 8 failures are emulation limits of the x86 emulator the container ran under: 5cctest/test-allocation/*OOMtests (deliberate ~128 TB allocations) and 3 mjsunit tests using--multi-mapped-mock-allocator(mremapwith old size 0).The repro numbers in https://github.com/acburdine/node-memory-reducer-repro were measured on v24.21.0 (100 s / 100 s / 141 s to the first memory-reducer GC vs 11 s with an earlier, embedder-only version of this fix).
Disclaimer
The investigation, the upstream CL and this backport were prepared with help from an AI coding assistant. The fix was reviewed and approved upstream by the V8 heap team; I reviewed the backport diff, and the verification above was run locally on this branch.