fix(gc): make BytesAllocated reads tear-free on 32-bit targets - #1214
Merged
Conversation
Owner-thread readers of the live-byte total took no lock while a cross-thread ReleaseExternalBytes (an error object charged on this collector but destroyed on another thread) writes it under the per-collector accounting lock. On i386-win32 a bare Int64 load can tear across that locked 64-bit write, feeding a garbage total into memory-limit and memory-pressure decisions. BytesAllocated now reads through an accessor that is a bare inlined field load on 64-bit (aligned Int64 loads cannot tear there; the per-allocation path keeps zero added cost) and a lock-guarded load on 32-bit. The decisive unlocked read in NeedsMemoryPressureCollection routes through it behind the cheap early-exit guards, the charge path passes the total it just committed instead of re-reading under the lock, and CanAllocateNativeBytes decides from one snapshot instead of two independent reads. Peak and lifetime totals stay direct reads: they are written only by the owning thread. The exposure predates the per-collector accounting lock (these readers never took the old global collect lock either), so this is correctness hardening, not a regression fix.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthroughThe garbage collector now uses platform-aware reads for ChangesAllocation accounting synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change makes cross-thread memory accounting reads safe on 32-bit targets while preserving the existing 64-bit fast path, with targeted tests and documentation updates. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description follows the required Summary and Testing sections. It explains the implementation, constraints, non-goals, issue context, documentation update, and test results. The optional benchmark item is unchecked, which does not prevent approval. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (3 skipped: 3 unsupported.) Comment |
Contributor
Web Tooling Benchmark
18 pinned Web Tooling workloads; 18 workloads produced at least one Goccia sample. Raw results from 1 sample per workload; full stdout/stderr for failures and min/max/CV stay in the |
Contributor
Suite TimingTest Runner (interpreted: 12,780 passed; bytecode: 12,780 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 440; bytecode: 440)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Boot
Empty-script ( Measured on ubuntu-latest x64. |
Contributor
Benchmark Results440 benchmarks · PR vs same-runner Interpreted: 🟢 27 improved · 🔴 35 regressed · 378 unchanged · avg +0.3% Typical per-run noise (median variance): interpreted ±2.0%, bytecode ±1.6%. Deltas within noise overlap and read as unchanged. arraybuffer.js — Interp: 🟢 1, 13 unch. · avg +1.0% · Bytecode: 🔴 1, 13 unch. · avg -1.9%
arrays.js — Interp: 🟢 1, 18 unch. · avg +1.7% · Bytecode: 🟢 1, 18 unch. · avg +1.1%
async-await.js — Interp: 6 unch. · avg -1.5% · Bytecode: 6 unch. · avg +0.3%
async-generators.js — Interp: 2 unch. · avg +1.9% · Bytecode: 2 unch. · avg -5.4%
atomics.js — Interp: 🔴 2, 4 unch. · avg -5.0% · Bytecode: 🟢 3, 🔴 1, 2 unch. · avg +1.2%
base64.js — Interp: 🟢 1, 9 unch. · avg +1.1% · Bytecode: 🔴 1, 9 unch. · avg -0.4%
classes.js — Interp: 🟢 2, 🔴 1, 28 unch. · avg -0.2% · Bytecode: 🟢 5, 🔴 2, 24 unch. · avg +0.1%
closures.js — Interp: 11 unch. · avg +1.5% · Bytecode: 11 unch. · avg +3.6%
collections.js — Interp: 🔴 1, 11 unch. · avg -2.5% · Bytecode: 🟢 1, 11 unch. · avg +1.2%
csv.js — Interp: 13 unch. · avg -1.2% · Bytecode: 13 unch. · avg -1.5%
destructuring.js — Interp: 🔴 1, 21 unch. · avg +0.7% · Bytecode: 🟢 1, 🔴 2, 19 unch. · avg +1.3%
fibonacci.js — Interp: 🔴 1, 7 unch. · avg -1.4% · Bytecode: 🟢 1, 🔴 2, 5 unch. · avg +0.8%
float16array.js — Interp: 🟢 2, 🔴 4, 26 unch. · avg +0.2% · Bytecode: 🟢 2, 🔴 1, 29 unch. · avg -0.1%
for-in/for-in.js — Interp: 3 unch. · avg +5.0% · Bytecode: 3 unch. · avg +0.3%
for-of.js — Interp: 🟢 1, 6 unch. · avg +1.8% · Bytecode: 🟢 1, 6 unch. · avg +0.9%
generators.js — Interp: 🟢 1, 3 unch. · avg +0.7% · Bytecode: 🟢 3, 1 unch. · avg +1.9%
intl.js — Interp: 6 unch. · avg +1.5% · Bytecode: 🔴 2, 4 unch. · avg -2.1%
iterators.js — Interp: 🟢 1, 🔴 4, 37 unch. · avg -1.6% · Bytecode: 🟢 7, 🔴 2, 33 unch. · avg +1.6%
json.js — Interp: 🟢 1, 🔴 2, 20 unch. · avg -0.6% · Bytecode: 🟢 4, 🔴 3, 16 unch. · avg +0.2%
jsx.jsx — Interp: 🟢 2, 19 unch. · avg +1.3% · Bytecode: 🔴 4, 17 unch. · avg -1.6%
modules.js — Interp: 🟢 1, 8 unch. · avg +1.6% · Bytecode: 9 unch. · avg +2.2%
numbers.js — Interp: 🔴 1, 11 unch. · avg -2.1% · Bytecode: 🟢 1, 11 unch. · avg -0.8%
objects.js — Interp: 8 unch. · avg +1.2% · Bytecode: 🟢 1, 🔴 1, 6 unch. · avg +2.0%
promises.js — Interp: 12 unch. · avg -1.6% · Bytecode: 12 unch. · avg -2.9%
property-access.js — Interp: 🟢 2, 3 unch. · avg +5.6% · Bytecode: 5 unch. · avg +2.2%
regexp.js — Interp: 🟢 3, 10 unch. · avg +2.8% · Bytecode: 🟢 2, 🔴 3, 8 unch. · avg +0.6%
strings.js — Interp: 🔴 1, 18 unch. · avg -0.7% · Bytecode: 🟢 2, 17 unch. · avg +3.0%
temporal.js — Interp: 🔴 1, 5 unch. · avg -0.5% · Bytecode: 6 unch. · avg -0.4%
tsv.js — Interp: 🟢 1, 8 unch. · avg -1.0% · Bytecode: 🟢 1, 🔴 1, 7 unch. · avg -1.2%
typed-arrays.js — Interp: 🔴 11, 11 unch. · avg -9.3% · Bytecode: 🟢 6, 🔴 3, 13 unch. · avg +5.2%
uint8array-encoding.js — Interp: 🟢 4, 🔴 5, 9 unch. · avg +9.7% · Bytecode: 🟢 3, 🔴 4, 11 unch. · avg -0.6%
weak-collections.js — Interp: 🟢 3, 12 unch. · avg +9.9% · Bytecode: 🟢 6, 🔴 3, 6 unch. · avg +15.0%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Each PR run also builds the |
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+0 / -0 / timeout +0 / -5)Resolved timeouts (5):
Steady-state failures and timeouts are non-blocking; PASS → non-timeout failure transitions fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
Contributor
JetStream 3 Performance Barometer
Geomean reference ratio: QuickJS 26.55×; Node.js 304.77×. 1.00× means aligned; values above 1.00× mean Goccia was proportionally slower after normalizing JetStream’s higher-is-better score. This is a directional barometer across runtimes with different goals, not a product ranking. Raw samples and failure details remain in the |
Contributor
AWFY Results
Geomean Ratios
14 pinned AWFY benchmarks. Medians from 5 interleaved samples per engine; raw JSON includes min/max/CV and is attached as the |
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
1 participant
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.
Summary
BytesAllocatedtotal: owner-thread readers take no lock, so a bareInt64load can tear across a cross-threadReleaseExternalBytes(an error object charged on this collector but destroyed on another thread) writing the counter under the per-collector accounting lock introduced in perf(gc): keep worker allocations running during another worker's collection #1213.BytesAllocatednow reads through an accessor: a bare inlined field load on 64-bit (alignedInt64loads cannot tear there — the per-allocation path keeps zero added cost) and a lock-guarded load on 32-bit. No 64-bit interlocked read is usable: FPC 3.2.2 declares those only underCPU64, and CI builds i386-win32.NeedsMemoryPressureCollectionroutes through the accessor behind the cheap early-exit guards (the periodic VM/interpreter pressure polls stay lock-free when no limit is set), the external-byte charge path decides from the total it just committed instead of re-reading under the lock, andCanAllocateNativeBytesdecides from one snapshot instead of two independent reads.PeakBytesAllocated,TotalBytesAllocated,MaxBytes, and the since-GC external counter stay direct field reads — they are written only by the owning thread, so no reader can observe a half-applied write. Compound predicates that consult the counter and then act remain approximate, as before.The reader rule is documented at
FAccountingLockand indocs/garbage-collector.md.Reviewed by a separate high-effort code-review pass: no correctness findings; its two 32-bit lock-traffic findings are fixed here (pressure-poll guards, single-snapshot
CanAllocateNativeBytes).Testing
--mode=astand--mode=bytecodedocs/garbage-collector.mdthreading modelGoccia.GarbageCollector.Test12/12 (incl. the cross-thread release churn test),Goccia.MemoryLimit.Test33/33; the 32-bit locked branch was additionally compiled and run on the 64-bit host by temporarily inverting the width gateFull CI (incl. the i386-win32 job, which
pr.ymldoes not run) dispatched on this branch: https://github.com/frostney/GocciaScript/actions/runs/32823875897