Repository navigation
Make -Xmx cap the whole heap under Node - #50
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 17 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
![project516-review-bot[bot]](https://avatars.githubusercontent.com/in/4903871?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR makes -Xmx cap the whole heap under Node by tracking estimated allocation sizes (32 bytes per object, byte length for arrays) and periodically checking actual V8 heap usage (heapUsed + arrayBuffers) against the cap, throwing OutOfMemoryError: Java heap space when live use stays over the cap after optional GC. The browser retains the per-array limit. Tests pass and performance impact is negligible.
review-bot, model nvidia/nemotron-3-ultra-550b-a55b:free, verdict approve
There was a problem hiding this comment.
This PR makes -Xmx cap the total live heap under Node by adding a sampled allocation check that compares process.memoryUsage().heapUsed + arrayBuffers against the cap after a baseline is taken post-boot. A new test HeapCap.java verifies catching and recovering from OutOfMemoryError. However, the module-level explicitMaxHeap flag is set to true when any VM uses -Xmx and never reset, so subsequent VMs created in the same process (via runNode) incorrectly inherit the heap cap check with the last-set MAX_HEAP value.
Not approved: 5 earlier points are still open in the threads.
review-bot, model nvidia/nemotron-3-ultra-550b-a55b:free, verdict comment
| // Reported by Runtime.maxMemory and the per-array limit. Node adds a sampled | ||
| // live-heap check when -Xmx is set; browser allocations keep the per-array cap. | ||
| export let MAX_HEAP = 1 << 30; | ||
| let explicitMaxHeap = false; |
There was a problem hiding this comment.
explicitMaxHeap is set to true in setMaxHeap (line 383) but never reset to false. If a process creates a VM with -Xmx (setting explicitMaxHeap = true) and later creates a VM without -Xmx, hasMaxHeap() still returns true and nodeVm enables the allocation check with the previous MAX_HEAP value. This violates the documented behavior that "No -Xmx: no check". Reset explicitMaxHeap when setMaxHeap is called without a cap, or add a clearMaxHeap function called when a VM starts without -Xmx.
| const toVfs = (p: string) => path.resolve(p); | ||
| const classpath = o.jar ? [toVfs(o.jar)] : o.classpath.map(toVfs); | ||
| const natives = o.natives ?? {}; | ||
| if (hasMaxHeap()) { |
There was a problem hiding this comment.
Because hasMaxHeap() relies on the one-way explicitMaxHeap flag, a VM started without -Xmx after one with -Xmx incorrectly enters this branch and installs an allocation check using the stale MAX_HEAP. The else clearAllocationCheck() branch is never reached for subsequent VMs. This should be fixed together with the explicitMaxHeap reset.
-Xmxnow caps the whole heap under Node. Before, it only capped a single array, so a program that kept many small objects grew until the RSS watchdog killed it with exit 137, where HotSpot throwsOutOfMemoryError: Java heap space, which a program can catch.How it works (
src/runtime.ts):-Xmxset, allocations add an estimate of their size to a counter (arrays by their bytes, objects a fixed 32). Nothing else is tracked per object.heapUsed + arrayBuffersis compared with the cap, measured from a baseline taken after boot so the JDK's own footprint does not count.gcis exposed (thejava-webcommand exposes it) and measures again; only if still over does that allocation throwOutOfMemoryError("Java heap space"). Withoutgc, it checks only after the estimate itself passes the cap, to avoid false errors.-Xmx: no check, as before (the RSS watchdog stays the backstop). The browser keeps the per-array limit.Cost: none measurable.
pnpm testtook 297 s against 293 s on master with one more test; javac under-Xmx512mtook 11.7 s against 12.1 s.Tests:
test/java/HeapCap.java(-Xmx32m) fills the heap with 256 KB arrays, catches theOutOfMemoryError, drops them and allocates again, matching HotSpot, with and without an exposedgc. Programs with-Xmxnow run in their ownjava-webprocess intest/programs.test.ts, since the cap is per process.HeapandJavacmatch HotSpot under-Xmx512m, so no false errors.QuadCurveOOMBugandUnboundValueAfterOOMEstill pass.pnpm typecheckandpnpm testpass.Noticed, not fixed: