refactor(engine): migrate per-thread cache clears into the cleanup registry (#893)#903
Merged
Conversation
…gistry (#893) Follow-up to #885/#891. PR #891 added Goccia.ThreadCleanupRegistry for the ~64 member-definition threadvars but left seven per-thread cache/memo clears as explicit calls inside Goccia.Threading.ShutdownThreadRuntime, leaving two cleanup idioms coexisting on the worker-exit path and coupling Goccia.Threading to seven cache-owning units. Each unit now registers its clear with the registry from its own initialization (matching the established pattern), and ShutdownThreadRuntime drops the explicit calls — keeping only the RunThreadvarCleanups drain before the ordered GC/CallStack/MicrotaskQueue shutdowns. Goccia.Threading no longer uses any of the seven units. - Atomics: register the per-thread ShutdownAtomicsWaitersForCurrentThread; keep the all-threads ShutdownAtomicsWaiters + lock teardown in the unit finalization (per-thread/all-threads distinction preserved); guard the per-thread proc against the registry's post-teardown main-thread drain. - TextSemantics (generic shared infra): register its is-ASCII memo clear from Goccia.RegExp.VM to keep the shared unit engine-free, and retain its own main-thread finalization for binaries that link it without the regex VM. - Broaden the registry callback contract to cover a thread's own entries in a shared lock-guarded structure (Atomics), not only managed threadvars. - Add a Goccia.ThreadCleanupLeak.Test gate pinning each of the seven registrations, plus ADR 0080. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ed28a # Conflicts: # docs/adr/README.md
|
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 (1)
📝 WalkthroughWalkthroughSeven per-thread cache and memo cleanup callbacks are registered during unit initialization, ChangesRegistry-driven thread cleanup migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/units/Goccia.RegExp.VM.pas (1)
1070-1073: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale memo cleanup comment.
This still describes the old finalization-only cleanup and bounded worker residual; the memo is now registered with
Goccia.ThreadCleanupRegistryand cleared on worker exit.Proposed comment update
-// FPC does not auto-finalize managed threadvars at thread exit, so the unit -// finalization below clears the main-thread memo on shutdown; a worker thread's -// last-held pair is a bounded residual, the same as the engine's other managed -// threadvars (e.g. each builtin's FStaticMembers). +// FPC does not auto-finalize managed threadvars at thread exit, so +// ClearRegExpInputMemo is registered with Goccia.ThreadCleanupRegistry to clear +// the calling thread's retained pair on worker exit and main-thread shutdown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.RegExp.VM.pas` around lines 1070 - 1073, Update the stale memo cleanup comment in the unit finalization area to match the current behavior: the memo is no longer cleared only by main-thread finalization, but is now registered with Goccia.ThreadCleanupRegistry and cleared on worker exit. Locate the comment near the memo cleanup logic in Goccia.RegExp.VM.pas and revise it so it accurately describes the registry-based cleanup path and no longer mentions the old bounded worker residual behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@source/units/Goccia.RegExp.VM.pas`:
- Around line 1070-1073: Update the stale memo cleanup comment in the unit
finalization area to match the current behavior: the memo is no longer cleared
only by main-thread finalization, but is now registered with
Goccia.ThreadCleanupRegistry and cleared on worker exit. Locate the comment near
the memo cleanup logic in Goccia.RegExp.VM.pas and revise it so it accurately
describes the registry-based cleanup path and no longer mentions the old bounded
worker residual behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d96e5ea-6c4a-4383-871c-b396109878ab
📒 Files selected for processing (12)
docs/adr/0081-migrate-cache-clears-into-cleanup-registry.mddocs/adr/README.mdsource/shared/TextSemantics.passource/units/Goccia.Builtins.Atomics.passource/units/Goccia.Builtins.DisposableStack.passource/units/Goccia.Builtins.Semver.passource/units/Goccia.ImportMeta.passource/units/Goccia.RegExp.VM.passource/units/Goccia.Temporal.TimeZone.passource/units/Goccia.ThreadCleanupLeak.Test.passource/units/Goccia.ThreadCleanupRegistry.passource/units/Goccia.Threading.pas
Contributor
Suite TimingTest Runner (interpreted: 11,046 passed; bytecode: 11,046 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: 437; bytecode: 437)
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.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results437 benchmarks · PR vs same-runner Interpreted: 🟢 32 improved · 🔴 24 regressed · 381 unchanged · avg +1.6% Typical per-run noise (median variance): interpreted ±3.0%, bytecode ±2.0%. Deltas within noise overlap and read as unchanged. arraybuffer.js — Interp: 🟢 1, 13 unch. · avg +1.2% · Bytecode: 🟢 2, 🔴 1, 11 unch. · avg -1.9%
arrays.js — Interp: 🔴 2, 17 unch. · avg +1.8% · Bytecode: 🟢 3, 🔴 3, 13 unch. · avg -1.1%
async-await.js — Interp: 6 unch. · avg -2.2% · Bytecode: 6 unch. · avg +1.8%
async-generators.js — Interp: 2 unch. · avg -1.9% · Bytecode: 2 unch. · avg +4.5%
atomics.js — Interp: 6 unch. · avg +1.7% · Bytecode: 🟢 2, 4 unch. · avg +2.5%
base64.js — Interp: 🔴 1, 9 unch. · avg -0.5% · Bytecode: 🟢 4, 6 unch. · avg +2.6%
classes.js — Interp: 🔴 3, 28 unch. · avg +0.0% · Bytecode: 🟢 2, 🔴 1, 28 unch. · avg +0.1%
closures.js — Interp: 🟢 2, 9 unch. · avg +1.5% · Bytecode: 🟢 2, 9 unch. · avg +2.0%
collections.js — Interp: 12 unch. · avg +1.5% · Bytecode: 🟢 3, 9 unch. · avg +2.8%
csv.js — Interp: 🔴 1, 12 unch. · avg +0.1% · Bytecode: 🟢 1, 🔴 1, 11 unch. · avg +2.1%
destructuring.js — Interp: 🔴 2, 20 unch. · avg -1.4% · Bytecode: 🟢 6, 16 unch. · avg +1.3%
fibonacci.js — Interp: 🟢 1, 7 unch. · avg +2.0% · Bytecode: 8 unch. · avg +2.2%
float16array.js — Interp: 🟢 5, 27 unch. · avg +2.4% · Bytecode: 🟢 6, 🔴 2, 24 unch. · avg +1.1%
for-in/for-in.js — Interp: 🟢 1, 2 unch. · avg +2.2% · Bytecode: 3 unch. · avg +1.3%
for-of.js — Interp: 🟢 4, 3 unch. · avg +3.9% · Bytecode: 🟢 1, 🔴 1, 5 unch. · avg -0.7%
generators.js — Interp: 4 unch. · avg +0.4% · Bytecode: 4 unch. · avg +1.5%
intl.js — Interp: 🔴 3, 3 unch. · avg -1.9% · Bytecode: 🟢 2, 4 unch. · avg +5.5%
iterators.js — Interp: 🟢 1, 🔴 6, 35 unch. · avg -1.6% · Bytecode: 🟢 2, 🔴 4, 36 unch. · avg +1.3%
json.js — Interp: 🟢 1, 🔴 2, 20 unch. · avg -0.2% · Bytecode: 🟢 3, 🔴 6, 14 unch. · avg -2.2%
jsx.jsx — Interp: 21 unch. · avg +0.7% · Bytecode: 🟢 9, 12 unch. · avg +4.9%
modules.js — Interp: 9 unch. · avg +2.2% · Bytecode: 9 unch. · avg +0.1%
numbers.js — Interp: 12 unch. · avg +0.7% · Bytecode: 🟢 3, 9 unch. · avg +5.1%
objects.js — Interp: 🟢 1, 6 unch. · avg +4.2% · Bytecode: 7 unch. · avg -1.0%
promises.js — Interp: 🟢 1, 11 unch. · avg +0.4% · Bytecode: 🟢 1, 11 unch. · avg +3.2%
property-access.js — Interp: 🟢 1, 4 unch. · avg +2.3% · Bytecode: 5 unch. · avg +0.7%
regexp.js — Interp: 🟢 1, 🔴 1, 9 unch. · avg +6.4% · Bytecode: 🔴 2, 9 unch. · avg +3.2%
strings.js — Interp: 🟢 4, 15 unch. · avg +1.5% · Bytecode: 🟢 5, 14 unch. · avg +3.4%
temporal.js — Interp: 🟢 1, 5 unch. · avg -1.1% · Bytecode: 🟢 1, 5 unch. · avg +4.2%
tsv.js — Interp: 9 unch. · avg +1.0% · Bytecode: 🟢 2, 7 unch. · avg +0.8%
typed-arrays.js — Interp: 🟢 6, 🔴 1, 15 unch. · avg +20.6% · Bytecode: 🟢 2, 🔴 5, 15 unch. · avg -2.2%
uint8array-encoding.js — Interp: 🔴 2, 16 unch. · avg -1.9% · Bytecode: 🟢 4, 🔴 2, 12 unch. · avg +8.8%
weak-collections.js — Interp: 🟢 1, 14 unch. · avg -0.9% · Bytecode: 🟢 5, 🔴 1, 9 unch. · avg +15.9%
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 (+1 / -1)Newly failing (1):
Newly passing (1):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) 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 |
…ed28a # Conflicts: # docs/adr/README.md
The memo declaration comment still described the pre-registry lifecycle: it claimed the unit finalization clears the main-thread memo and that a worker thread's last-held pair is a bounded residual. After the #893 migration the memo is registered with Goccia.ThreadCleanupRegistry, so the drain clears it on worker exit and at main-thread shutdown — no residual. Comment-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
Completes the thread-cleanup migration that ADR 0078 deferred to this issue. #891 added
Goccia.ThreadCleanupRegistryfor the ~64 member-definition threadvars but left seven per-thread cache/memo clears as explicit calls insideGoccia.Threading.ShutdownThreadRuntime(ClearImportMetaCache,ShutdownAtomicsWaitersForCurrentThread,ClearDisposableStackSlotMap,ClearSemverHosts,ClearTimeZoneCache,ClearRegExpInputMemo,ClearAsciiMemo) — two cleanup idioms on the worker-exit path, and auses-clause couplingGoccia.Threadingto seven cache-owning units.Each unit now registers its clear with the registry from its own
initialization(the established pattern), andShutdownThreadRuntimedrops the seven explicit calls — keeping only theRunThreadvarCleanupsdrain before the ordered GC / CallStack / MicrotaskQueue shutdowns (soClearImportMetaCache's GC unpin still runs while the collector is alive).Goccia.Threadingno longerusesany of the seven units.Key implementation decisions (also recorded in ADR 0083):
ShutdownAtomicsWaitersForCurrentThreadis registered for the worker path; the unit's ownfinalizationkeeps the all-threadsShutdownAtomicsWaiters+ lock teardown for the main thread (the per-thread/all-threads distinction the issue calls out). Because the registry finalizes after this unit, its main-thread drain would call the per-thread proc after the lock is destroyed — a pre-lockif not Assigned(GAtomicsWaiters)guard makes that a safe no-op. The registry's callback contract is broadened to cover "a thread's own entries in a shared lock-guarded structure."TextSemanticsis genericsource/shared/infrastructure with no engine dependency. To keep it engine-free, its is-ASCII memo clear is registered fromGoccia.RegExp.VM(every engine binary links the regex VM) for the worker path, and its own main-threadfinalizationis retained — the JSON/numeric/config tools andTextSemantics.Testlink it without the regex VM and would otherwise leak the memo on the main thread.ImportMetakeepsClearImportMetaCacheexported (the engine also calls it directly during teardown).Non-goals: no runtime behaviour or conformance change — this is the cleanliness/embeddability follow-up, not a perf change. The symbol-registry threadvar audit (#892) stays a separate follow-up.
A new
Goccia.ThreadCleanupLeak.Testgate pins each of the seven registrations via a smallIsThreadvarCleanupRegisteredhelper, so a droppedRegisterThreadvarCleanupfails loudly. The branch is merged up toorigin/main; the ADR was renumbered to 0083 after #899/#902 landed ADRs 0080–0082 concurrently.Closes #893
Testing
Verification (clean build, post-merge with
origin/main):./build.pas testrunner && ./build/GocciaTestRunner tests→ 11046/11046 (AST)./build/GocciaTestRunner tests --mode=bytecode→ 11046/11046 (bytecode)Goccia.ThreadCleanupLeak.Test(with the newTestMigratedCacheCleanupsAreRegistered) andGoccia.Threading.Test(ShutdownThreadRuntimedrains the registry once per worker), plus the shared-consumer testsTextSemantics.Test/JSONParser.Test/NumericText.Test/CLI.ConfigFile.Test../format.pas --check→ clean (376 files).Not benchmarked: no hot-path or allocation change (teardown-only plumbing).