Skip to content

perf: scoped context cache invalidation & directive dispatch optimizations#282

Open
evertonfraga wants to merge 4 commits into
no-js-dev:mainfrom
evertonfraga:perf/runtime-optimizations
Open

perf: scoped context cache invalidation & directive dispatch optimizations#282
evertonfraga wants to merge 4 commits into
no-js-dev:mainfrom
evertonfraga:perf/runtime-optimizations

Conversation

@evertonfraga

Copy link
Copy Markdown

Summary

Runtime performance optimizations targeting the two hottest paths in No.JS: expression evaluation (context key collection) and directive dispatch (DOM tree walking).

Changes

Context cache invalidation — 5.8× eval speedup (src/context.js)

The _collectKeys cache was invalidated on every reactive mutation anywhere in the app (global generation counter). This meant unrelated context writes blew caches for all contexts — catastrophic for pages with many independent reactive scopes.

Fix: Each context now tracks its own _gen stamp. Cache validation walks the ancestor chain comparing per-context stamps (O(depth) integer compares, depth typically 1–5) instead of relying on a single global counter. Unrelated mutations no longer invalidate unrelated caches.

Directive dispatch fast-path (src/registry.js)

  • Replaced has() + get() double-lookup with a single Map.get() for exact directive matches
  • Added a first-char guard (c/o/s/b) to skip the wildcard pattern loop entirely for plain HTML attributes (id, href, data-*, aria-*, etc.) — the overwhelmingly common case
  • Skip sort when only 0–1 directives matched
  • Replaced for..of with indexed for loops in hot paths

DOM walk guard (src/registry.jsprocessTree)

  • Replaced root.contains(node) (O(depth) per node) with node.isConnected (O(1)) for live documents
  • Skip the connectivity check entirely for DocumentFragment roots

Loop keyed-reuse cache fix (src/directives/loops.js)

When reusing keyed elements via Object.assign (bypassing the proxy setter), the _collectKeys cache was not invalidated. Added explicit cache deletion on the reuse path.

Minor

  • _hasGlobals fast guard to skip for..in over empty _globals in evaluate (common case — no plugins)
  • Benchmark test repairs for marker-based loop architecture

Testing

  • Existing unit tests pass
  • Loops reconciliation benchmarks repaired and verified
  • Manual verification of the 5.8× eval speedup claim via isolated bench

…sed loops

The loops/reconciliation benchmark accessed `list.parentElement.__ctx` and
`list.children`, but since the v1.15 marker-based loop refactor the loop
element is removed from the DOM (replaced by comment markers) and clones are
managed as siblings of the parent `state` element. `list.parentElement` was
therefore null, so all 96 tests threw `Cannot read properties of null
(reading '__ctx')` against v1.16.1 and the benchmark produced zero data.

Point state mutations and assertions at the `state` element (the context
host) instead. Drop the now-unused `list` destructure in test bodies;
setupDOM still returns it for callers that want the template element.

No production code changed.
Three targeted optimizations in the init/processTree hot path:

1. _matchDirective: use a single Map.get instead of has+get (eliminates
   double hash lookup on every exact match), and add a charCodeAt(0) gate
   so plain-HTML attributes (id, href, data-*, aria-*) skip the 4-way
   startsWith pattern loop entirely.

2. processElement: early-return for zero-directive elements (the vast
   majority on any page), skip sort() for ≤1 match, and use an index
   loop instead of for..of iterator allocation.

3. processTree: replace root.contains(node) — an O(depth) ancestor walk
   per element — with the O(1) node.isConnected check, gated on
   root.isConnected to preserve correct processing of DocumentFragment
   roots.
The keyed reconciliation update branch (loops.js:412) mutates __raw via
Object.assign, bypassing the proxy set trap. Unlike its three sibling
paths (filter at :187, delta-append at :297, key-eval at :379), it did
NOT invalidate __collectKeysCache. This meant the reused child context
could serve stale cached keys on re-render.

Currently masked by the global _ctxGeneration counter (any other reactive
write bumps it), but this becomes a live correctness bug when scoped
invalidation (R1) is implemented.

Also: single Map.get instead of double lookup, add _hasGlobals flag to
globals.js (for future context.js optimization).
Replace the single global _ctxGeneration counter with per-context _gen
stamps. _collectKeys now validates its cache against the self + ancestor
chain's generations — unrelated context writes no longer blow the cache.

Before: any reactive write anywhere bumped a global counter, forcing
EVERY context's _collectKeys to recompute on next evaluate(). Verified
penalty: 5.8x slower evals (0.54µs → 3.13µs) when an unrelated region
updated concurrently.

After: eval + unrelated write = 0.544µs (ratio 0.92x of warm baseline).
The cache is only invalidated when the context itself or one of its
actual ancestors mutates. Global _ctxGeneration is retained for backward
compat with the `delete __collectKeysCache` paths in loops.js (which
explicitly bust the cache when bypassing the proxy setter).

Correctness verified: self-write invalidates, parent-write invalidates
child, unrelated-write preserves cache. All 1914 unit tests pass.
@ErickXavier

Copy link
Copy Markdown
Collaborator

Review: PR#282 — scoped context cache invalidation & directive dispatch optimizations

Full review of the four commits, plus unit + e2e runs on the branch and a merge dry-run against current main. TL;DR: the core idea (per-context __ctxGen stamps instead of a global generation) is sound and still worth doing, but this branch is based on pre-v1.19.0 main and no longer merges, it ships one confirmed correctness bug in its own new invalidation scheme, and it's missing dist/, tests, and some cleanup. Recommendation at the bottom.


Test results on this branch

  • Unit (Jest): ✅ 27 suites, 1914 passed, 3 skipped.
  • E2E (Playwright): ❌ 13 failed / 469 passed / 46 skipped — insert-modes 1, 2, 4 on all three browsers, animations stagger on all three browsers, pagination 4 on chromium.

The e2e failures are not caused by this PR's source changes. Two pieces of evidence:

  1. The e2e fixtures load ../../dist/iife/no.js, and this PR never rebuilds dist/ — so the branch's e2e run exercises the old committed build, not the PR source at all.
  2. Bisecting in a worktree: the same failures reproduce at 569c84f (the benchmark-only commit, whose src/ is identical to the merge-base), and all 16 relevant chromium tests pass on current main. The failing specs were all fixed on main after this branch diverged: 6a17699 (insert-modes rewrite to JSON data-mode contract), 8e4e529 (stagger fixture fix), 9227fa3 (pagination spec 4 deflake).

So the e2e signal here is stale-base noise — but that leads directly to the biggest problem.


1. 🚫 Blocker: branch is based on stale main and does not merge

Merge-base is 4e9ccd2before v1.19.0. Since then, main landed the WS1–WS5 reactive-core perf overhaul touching exactly the files this PR optimizes:

  • 2769d01 — shared Proxy handler, interned keys (rewrites context.js)
  • b82032f — compiled expression closure trees (rewrites evaluate.js)
  • 92d8473 — per-template process plans for loop clones (rewrites loops.js; clones are now processed via _processClone after insertion)
  • 5e3a880, 744f905, 9057d1d — disposal, reconciliation, and post-merge fixes
  • b88d627__benchmarks__/ untracked on main entirely

A dry-run merge of this branch into current main conflicts in src/context.js, src/directives/loops.js, src/registry.js, plus a modify/delete conflict on __benchmarks__/loops-benchmark.test.js.

The good news: main's _collectKeys still uses the global _ctxGeneration compare, so the scoped-invalidation win this PR targets is still available on main. But a mechanical rebase would risk silently reverting parts of the WS overhaul in the same hot paths. Suggest re-implementing the two ideas (per-context gen stamps; _matchDirective / processTree dispatch tweaks) as a fresh branch off current main, carrying over the fixes below.

2. 🐛 Confirmed bug: keyed-reuse raw write doesn't bump __ctxGen → descendant caches serve stale values

src/directives/loops.js (keyed reconcile reuse path):

const reused = keyMap.get(key);
Object.assign(reused.__ctx.__raw, childData);
// Invalidate _collectKeys cache since we bypassed the proxy setter
delete reused.__ctx.__raw.__collectKeysCache;
reused.__ctx.$notify();

This deletes only the reused context's own cache, but the new scheme validates a cache against the __ctxGen of every ancestor in the chain (gens[] snapshot in _collectKeys). Since Object.assign bypasses the set trap, the reused context's __ctxGen is never bumped — so any descendant context (nested foreach, nested state scope, template include) that snapshotted this context's gen still validates as fresh and returns stale inherited values.

Reproduced directly against the branch source:

const parent = createContext({ row: { name: 'OLD' } });
const child  = createContext({ cell: 1 }, parent);
_collectKeys(child);                                  // primes child's cache
Object.assign(parent.__raw, { row: { name: 'NEW' } }); // loops.js reuse pattern
delete parent.__raw.__collectKeysCache;
_collectKeys(child).vals.row.name;                     // → 'OLD'  ❌ stale

In the common flow this is masked: the original proxied list write (state.items = [...]) bumps the declaring context's gen, which sits in every loop-item descendant's chain, so their caches invalidate anyway. But any raw-write + $notify flow with no proxied ancestor write — the exact pattern loops.js itself uses at its four Object.assign-on-__raw sites (~lines 187, 297, 379, 414), and the pattern any plugin doing ctx.__raw.x = ...; ctx.$notify() uses — leaves nested contexts rendering stale data. Under the old global-generation scheme, any write anywhere eventually flushed these; the new precise scheme makes the staleness permanent until that specific context happens to get a proxied write.

Fix: replace the manual cache delete with a gen bump. Concretely: export a helper from context.js (e.g. _bumpCtxGen(ctx) that does ctx.__raw.__ctxGen++) and call it at all four loops.js bypass sites. That invalidates the context's own cache and every descendant's, restores one invalidation protocol everywhere, and removes 4 copy-pasted delete <raw>.__collectKeysCache lines that each hard-code a context.js private whose shape this very PR just changed ({gen,result}{gens,result}).

3. ⚠️ processTree: the narrowed guard loses removed-node protection for detached roots

const checkConnected = root.isConnected;
...
if (checkConnected && !node.isConnected) continue;

The old if (!root.contains(node)) continue; guarded two things the new check doesn't:

  • Detached roots (root.isConnected === false): the guard is now skipped entirely. The comment justifies this for DocumentFragments, but processTree is publicly exported and NoJS.init accepts a caller-supplied root, so a detached element subtree is a supported input. If that subtree contains a structural directive that removes nodes mid-walk — if="false" clears innerHTML (conditionals.js), each removes the template element — the already-collected descendant nodes get processElement()'d as orphans: contexts, watchers, and comment markers created on DOM that never gets inserted (double-init / leak). root.contains(node) used to skip exactly these.
  • Moved-but-still-connected nodes: a directive that relocates a node elsewhere in the document during init was previously skipped (!root.contains(node)); now node.isConnected is true and it's processed under the wrong root's walk. There's also the symmetric edge: a connected root that detaches itself mid-walk now skips all remaining (still valid) descendants.

Fix: keep the O(1) fast path for the common case but don't drop the guard for detached roots — e.g. const inDoc = root.isConnected; ... if (inDoc ? !node.isConnected : !root.contains(node)) continue;. That preserves the perf win where it matters (large connected trees) and the old semantics everywhere else.

4. 📦 dist/ not rebuilt (workspace convention)

Six src/ files change but no commit touches dist/. Workspace CLAUDE.md: "The built output must always reflect the current source — never commit stale or conflict-mangled dist files" (and Feature Delivery Flow step 4). Since the CDN serves dist/iife/no.js, merging as-is would ship none of these changes — and, as noted above, it's also why the branch's own e2e run silently tested the wrong code. Worth adding a CI check that dist/ is in sync with src/ at PR time, since this failure mode (green-looking e2e over a stale build) is invisible in review.

5. 🧪 No unit tests for the new invalidation logic

git log main..HEAD -- __tests__/ is empty. The core behavioral claims live only in commit messages:

  • unrelated context write preserves another context's _collectKeys cache (the 5.8× win),
  • own/ancestor write invalidates it,
  • the keyed-reuse bypass path invalidates it (the fix(loops) commit calls this "a live correctness bug"),
  • chain-shrink/grow validation edges.

Feature Delivery Flow step 2 requires Jest tests with ≥80% coverage on new code. Note the confirmed bug in item 2 is precisely the test that's missing — a describe('scoped cache invalidation') block with the four cases above (including a descendant-of-bypassed-context case) would have caught it and will protect the re-implementation on current main.

6. 🧹 Dead code and comments that don't match the code

  • _hasGlobals / _setHasGlobals (globals.js:42, index.js:33/337/377): maintained on register/dispose but never read. The new comments in evaluate.js (~1477, ~1518) claim "the for..in is skipped entirely when _globals is empty … _hasGlobals is maintained by index.js" — no such guard exists; both for..in loops run unconditionally (harmlessly — iterating an empty prototype-free object is already a no-op, which is also why the flag isn't worth wiring in). Suggest deleting the flag, its setter, the import and both call sites, and rewording the comments to "for..in over empty _globals iterates nothing".
  • _ctxGeneration (context.js:17): now write-only — bumped on every reactive set (lines 101, 169) and read nowhere after _collectKeys moved to per-context gens. Its comment says it's "kept for backward compatibility with any code that deletes __collectKeysCache (loops.js etc.)", but the delete path never consults it. Beyond the pointless bump on the hottest path, the comment invites someone to key future cache logic on a dead counter. Delete it (or actually wire it).
  • let _gen = 0 mirrors raw.__ctxGen (context.js:~51): every mutation site must bump both in lockstep (_gen++; raw.__ctxGen = _gen; duplicated in the set trap and the nested-$set branch). A single raw.__ctxGen++ (or one shared bump() closure used by both sites) removes the desync hazard.

7. 🔧 Smaller points

  • _matchDirective charCode guard (registry.js:56): hardcoding 99/111/115/98 (c/o/s/b) couples the fast path to the literal contents of _MATCH_PATTERNS. Currently safe (frozen list, plugins can't add wildcard patterns, exact-name lookup runs first), but a future wildcard family with a new leading letter (attr-*, text-*) that misses the guard would silently never match — no error, just dead directives. Derive it once at module load: const _PREFIX_HEADS = new Set(_MATCH_PATTERNS.map(p => p.prefix.charCodeAt(0)));.
  • Missing deleteProperty trap (context.js proxy handler): delete proxy.key removes the key from raw without bumping __ctxGen or notifying. Latent today (no internal caller), but the per-context scheme makes it worse than before: under the global generation, any write anywhere eventually flushed the stale cache; now nothing does until that specific context gets a proxied write. Cheap to add alongside this work.
  • Benchmark repair commit (569c84f): the liststate change is correct for marker-based loops (the each element is removed from the DOM, clones become element-children of the container, comment markers don't count toward children). No issues — but note main has since untracked __benchmarks__/, so this commit conflicts as modify/delete and should just be dropped in the re-do.
  • Verified non-issues, for the record: the processElement early-return / conditional sort / unconditional _devtoolsEmit restructure is behavior-identical to the old code; the _collectKeys chain shrink/grow validation handles both edges correctly; __ctxGen appearing in collected keys doesn't leak anywhere user-visible (devtools and persistence both filter __-prefixed keys).

Suggested path forward

  1. Close or repurpose this branch; start fresh from current main — all three conflicted files were rewritten there, and main's _collectKeys still has the global-generation compare, so the win is still on the table.
  2. Re-implement per-context __ctxGen with a single exported _bumpCtxGen(ctx) helper; make the loops.js raw-write sites call it instead of deleting __collectKeysCache; drop _ctxGeneration and the _gen mirror.
  3. Keep the _matchDirective and processTree dispatch optimizations, with the prefix set derived from _MATCH_PATTERNS and the detached-root guard preserved (inDoc ? !node.isConnected : !root.contains(node)).
  4. Add the invalidation unit tests (including the descendant-of-bypassed-context regression case) and a deleteProperty trap.
  5. Rebuild and commit dist/, then run e2e against the rebuilt bundle — on current main the previously failing specs are already fixed, so the suite should be green end to end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants