refactor(engine): unify embedded-data caches onto a lock-free publication primitive#902
Merged
Merged
Conversation
…tion primitive #813 gave the RegExp case-fold tables a barrier-correct double-checked publication, but the sibling embedded-data caches kept the old shapes: the TimeZone and CLDR resource readers were unsynchronized (cold-load data race plus a publication gap on weakly-ordered targets), while the UCD blob, the identifier ID_Start/ID_Continue ranges, and the available-locale list entered a critical section and copied their array out on every call. Introduce TLazyPublishedCache<T> (source/shared/LazyPublishedCache.pas): a generic record that bundles Data, the Loaded/Available flags and the Lock, and owns the lazy one-shot load plus barrier-correct lock-free publication in one Ensure(key, loader). All eight caches across five units now consume it via a small per-cache loader; hot readers (identifier ranges, case-fold pairs) read .Data in place via a const argument, so the lexer identifier path is now lock-free and zero-copy. The barrier discipline lives in one place and a mismatched (data, flag, lock) pairing is a compile-time error. Behavior-preserving: full JS suite passes in both interpreter and bytecode modes; TimeZone/CLDR additionally gain a cold-load lock and memoize load failure. The weak-memory ordering is not JS-testable, so the primitive's functional contract is locked in by the Pascal gate LazyPublishedCache.Test. The audit excludes the TimeZone threadvar caches (per-thread) and the ICU EnsureLoaded FFI binds (different shape). See ADR 0080. Closes #894 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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 (2)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughIntroduces ChangesTLazyPublishedCache primitive and migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
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: 🟢 25 improved · 🔴 49 regressed · 363 unchanged · avg -0.4% Typical per-run noise (median variance): interpreted ±3.0%, bytecode ±2.2%. Deltas within noise overlap and read as unchanged. arraybuffer.js — Interp: 🟢 1, 13 unch. · avg +1.0% · Bytecode: 🔴 1, 13 unch. · avg +0.5%
arrays.js — Interp: 🔴 2, 17 unch. · avg -0.6% · Bytecode: 🟢 1, 18 unch. · avg +0.6%
async-await.js — Interp: 6 unch. · avg +2.4% · Bytecode: 6 unch. · avg -1.1%
async-generators.js — Interp: 2 unch. · avg +2.7% · Bytecode: 2 unch. · avg +2.4%
atomics.js — Interp: 🟢 1, 5 unch. · avg -1.3% · Bytecode: 6 unch. · avg -0.6%
base64.js — Interp: 🟢 2, 🔴 1, 7 unch. · avg -0.6% · Bytecode: 10 unch. · avg +1.3%
classes.js — Interp: 🟢 4, 27 unch. · avg +0.5% · Bytecode: 🟢 2, 29 unch. · avg +0.2%
closures.js — Interp: 🔴 2, 9 unch. · avg -2.6% · Bytecode: 🔴 3, 8 unch. · avg +0.5%
collections.js — Interp: 🟢 1, 🔴 1, 10 unch. · avg +1.3% · Bytecode: 🟢 1, 11 unch. · avg +0.8%
csv.js — Interp: 🔴 1, 12 unch. · avg +0.8% · Bytecode: 13 unch. · avg +1.7%
destructuring.js — Interp: 🔴 4, 18 unch. · avg -0.3% · Bytecode: 🟢 1, 🔴 2, 19 unch. · avg +0.8%
fibonacci.js — Interp: 8 unch. · avg -2.6% · Bytecode: 🟢 2, 6 unch. · avg -0.2%
float16array.js — Interp: 🔴 9, 23 unch. · avg -3.6% · Bytecode: 🟢 3, 🔴 1, 28 unch. · avg +1.0%
for-in/for-in.js — Interp: 3 unch. · avg -3.0% · Bytecode: 3 unch. · avg -4.5%
for-of.js — Interp: 🔴 3, 4 unch. · avg -1.9% · Bytecode: 7 unch. · avg -1.5%
generators.js — Interp: 4 unch. · avg -0.1% · Bytecode: 4 unch. · avg -2.5%
intl.js — Interp: 🔴 1, 5 unch. · avg -3.1% · Bytecode: 🟢 1, 5 unch. · avg +0.6%
iterators.js — Interp: 🟢 3, 🔴 4, 35 unch. · avg -0.4% · Bytecode: 🟢 1, 🔴 8, 33 unch. · avg -1.7%
json.js — Interp: 🔴 3, 20 unch. · avg -1.8% · Bytecode: 🟢 3, 🔴 2, 18 unch. · avg +0.7%
jsx.jsx — Interp: 21 unch. · avg +0.1% · Bytecode: 🟢 3, 18 unch. · avg +3.3%
modules.js — Interp: 🔴 1, 8 unch. · avg -3.4% · Bytecode: 🔴 1, 8 unch. · avg -3.1%
numbers.js — Interp: 🟢 1, 11 unch. · avg -3.2% · Bytecode: 🔴 1, 11 unch. · avg +1.0%
objects.js — Interp: 🔴 2, 5 unch. · avg -6.6% · Bytecode: 7 unch. · avg +6.1%
promises.js — Interp: 🟢 2, 10 unch. · avg +0.9% · Bytecode: 🟢 1, 11 unch. · avg +3.4%
property-access.js — Interp: 🔴 1, 4 unch. · avg -2.1% · Bytecode: 5 unch. · avg -5.3%
regexp.js — Interp: 🔴 1, 10 unch. · avg -0.6% · Bytecode: 🔴 3, 8 unch. · avg -0.2%
strings.js — Interp: 🔴 1, 18 unch. · avg -3.3% · Bytecode: 🟢 4, 15 unch. · avg +2.3%
temporal.js — Interp: 6 unch. · avg -1.7% · Bytecode: 🔴 1, 5 unch. · avg -2.8%
tsv.js — Interp: 9 unch. · avg -1.0% · Bytecode: 9 unch. · avg -0.5%
typed-arrays.js — Interp: 🟢 7, 🔴 2, 13 unch. · avg +8.5% · Bytecode: 🟢 5, 🔴 3, 14 unch. · avg -0.6%
uint8array-encoding.js — Interp: 🔴 10, 8 unch. · avg -13.3% · Bytecode: 🟢 2, 🔴 5, 11 unch. · avg -13.8%
weak-collections.js — Interp: 🟢 3, 12 unch. · avg +19.9% · Bytecode: 🟢 2, 🔴 3, 10 unch. · avg +13.2%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Each PR run also builds the |
coderabbitai
Bot
added
internal
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/shared/LazyPublishedCache.Test.pas (1)
47-64: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAdd a cold-load contention test for the cache contract.
The suite covers sequential memoization, but not the PR’s core concurrency guarantee. Add a test where multiple threads call
Ensureon the same cold cache and assert the loader ran once and every reader sees initialized data.🤖 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/shared/LazyPublishedCache.Test.pas` around lines 47 - 64, The current TLazyPublishedCacheTests suite covers warm reads and memoization but misses the cold-load contention contract. Add a new test method in TLazyPublishedCacheTests, register it in SetupTests, and use the existing Ensure path on a shared cold cache from multiple threads to verify the loader executes only once and every caller observes initialized data. Reuse the cache’s published state and loader hooks so the test asserts both single execution and correct visibility under contention.
🤖 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.
Inline comments:
In `@source/shared/LazyPublishedCache.pas`:
- Around line 87-89: The shared cache publish path in LazyPublishedCache should
not let a failed loader leave partial data in the Data slot. Update the load
flow around the Available := ALoader(AKey, Data) call so the loader writes into
a temporary buffer and Data is assigned only when the load succeeds, or
explicitly clear Data before publishing a failure. Use the existing
LazyPublishedCache load/publish logic and the ALoader callback as the main
touchpoints.
---
Nitpick comments:
In `@source/shared/LazyPublishedCache.Test.pas`:
- Around line 47-64: The current TLazyPublishedCacheTests suite covers warm
reads and memoization but misses the cold-load contention contract. Add a new
test method in TLazyPublishedCacheTests, register it in SetupTests, and use the
existing Ensure path on a shared cold cache from multiple threads to verify the
loader executes only once and every caller observes initialized data. Reuse the
cache’s published state and loader hooks so the test asserts both single
execution and correct visibility under contention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 52b2494d-71a0-4fbe-bebe-012a634f83cf
📒 Files selected for processing (9)
docs/adr/0080-lazy-published-cache-primitive.mddocs/adr/README.mdsource/shared/IntlLocaleResolver.passource/shared/LazyPublishedCache.Test.passource/shared/LazyPublishedCache.passource/units/Goccia.Identifier.passource/units/Goccia.Intl.CLDRData.passource/units/Goccia.RegExp.UnicodeData.passource/units/Goccia.Temporal.TimeZoneData.pas
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+2 / -0)Newly passing (2):
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 |
…deleev-034991 # Conflicts: # docs/adr/README.md
CodeRabbit flagged that TLazyPublishedCache.Ensure let the loader write straight into the shared Data slot before the outcome was known: a loader that sized its payload and then failed (e.g. LoadTZResource sizing the byte buffer before a failing ReadBuffer) left that partial blob resident for the process lifetime, since Loaded stays True and the slot is never reloaded. Reads were already gated on Available, so this was a memory-hygiene gap rather than a correctness bug. Clear Data (Data := Default(T)) whenever the loader returns False, before the publish, so a failed load publishes an empty value. Fixing it in the primitive covers every consumer's loader rather than patching each resource reader. Add a LazyPublishedCache.Test case that fills a non-empty buffer then fails and asserts the published Data is empty. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…deleev-034991 # Conflicts: # docs/adr/README.md
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
TLazyPublishedCache<T>(source/shared/LazyPublishedCache.pas): a generic record that bundlesData+Loaded/Availableflags +Lockand owns the lazy one-shot load plus barrier-correct lock-free publication (double-checked:WriteBarrierbefore publishingLoaded, matchingReadBarrieron the warm read) in a singleEnsure(key, loader).ID_Start/ID_Continueranges, and the available-locale list. Each unit keeps a small per-cache loader; the barrier discipline now lives in one place and a mismatched(data, flag, lock)pairing is a compile-time error.#813/perf(regexp): serve case-fold tables lock-free on the icase hot path #895 idiom: previously TimeZone/CLDR were unsynchronized (cold-load data race + publication gap on weakly-ordered targets), while the UCD blob, identifier ranges, and locale list were always-locked and copied their array out per call. Hot readers (identifier ranges, case-fold pairs) now read.Datain place via aconstargument, so the lexer identifier path is lock-free and zero-copy.Data:Ensureclears the slot (Data := Default(T)) when the loader returnsFalse, so a resource read that sizes its buffer before a failingReadBuffercannot leave a stale blob resident (addresses CodeRabbit review).dmb ishld/dmb ishston AArch64 (verified) and to compiler barriers on x86 (TSO).IntlLocaleResolver(the available-locale list) is the same-shape cache surfaced by the issue's audit bullet, not named in the body/comment; included here and trivially separable. The audit excludesGoccia.Temporal.TimeZone'sthreadvarcaches (per-thread, no cross-thread publication) and the ICUEnsureLoadedFFI binds (library-load, not an immutable data table).Closes #894
Testing
origin/mainthrough perf(vm): unbox typed-array element reads and writes in the bytecode VM #900).language.md/built-ins.mdare unaffected.source/shared/LazyPublishedCache.Test.pas(load-once, memoize-failure, drops-partial-data-on-failure, key-passthrough, payload-agnostic) passes 5/5; full./build.pas testsbuilds clean../format.pas --check: 376 files clean.