perf(core): memoize member-lookup DeclMaps (pure builders) — cut 1 - #179
Merged
Conversation
…re builders) A repeated `BASE[member]` rebuilt its member index per access (measured: a detached-ruleset map read 5× rebuilt the DeclMap 10×). Cache the DeclMap on the resolving frame, keyed by the base node identity, for the PURE builders only — collection, namespace-selector, and detached-ruleset body (`resolveBaseDeclMap` Collection / Any-Keyword / resolveForRuleset arms). Plain Map, not a WeakMap: the frame owns the lifetime and disposes it with the render; a node-keyed WeakMap would pin DeclMaps for the AST's whole cross-render lifetime. Gated per-arm (never the Reference / MixinCall arms, which consult e.excluded or run expandCall); stored only when no alias cycle is being resolved (e.excluded empty), so a computed key over an actively-excluded alias is never cached; never caches null. Negative control: JESS_NO_DECLMAP_MEMO=1 reproduces the un-memoized counts. The mixin-call dispatch (`@p:.mk()[x]`) is deliberately NOT memoized here (it mutates the caller frame) — deferred to the second cut behind the scoping decision. Measured (negative control): detached map read 5× → DeclMap builds 10→1. Full core + jess suites byte-identical; ratchet updated (+1 fn, +1 Map; WeakMap stays 4). Design: docs/design/MIXIN-SCOPING-AND-LOOKUP-MEMO.md
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
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.
Repeated
BASE[member]lookups rebuilt their member index per access (measured: a detached-ruleset map read 5× rebuilt the DeclMap 10×). This caches the DeclMap on the resolving frame, keyed by base-node identity, for the pure builders only — collection, namespace-selector, and detached-ruleset body (the@usemodule path too). PlainMap, not a WeakMap: the frame owns the lifetime and disposes it with the render.Gated per-arm (never the
Reference/MixinCallarms, which consulte.excludedor runexpandCall); stored only when no alias cycle is active (e.excludedempty), so a computed key over an actively-excluded alias is never cached; never caches null. Negative control:JESS_NO_DECLMAP_MEMO=1reproduces the un-memoized path.Deliberately deferred: the mixin-call dispatch (
@p:.mk()[x]) is NOT memoized — it mutates the caller frame (leakBodyVars/publish), so memoizing it isn't observationally free. That's a second cut behind the mixin-scoping decision.Reviewed by perf-architecture twice (design + implementation): per-arm gating, store guard, WeakMap-free per-render lifetime, and node+frame keying all confirmed correct; byte-transparent on/off. Committed correctness lock:
lookup-declmap-memo.test.ts(passes identically with the memo on and off; a sibling-member-reference case proves the memo caches the index, not values). Full core + jess suites byte-identical; ratchet updated (+1 fn, +1 Map;new WeakMapunchanged at 4).Design:
docs/design/MIXIN-SCOPING-AND-LOOKUP-MEMO.md. Unblocks D18 (SCSS module member access) and is the general re-index fix; the caller-scope-leak close (R16) and the mixin-call second cut are tracked there for a later PR.