Intern Context.hover/active/focus/visit per element - #2168
Conversation
Context.focus(element) (and the other single-state static factories) constructed a brand-new Context instance on every call, even for the same element. Since Cascade/Style's caches key on Context object identity (WeakMap-backed), repeated single-element-context queries for the same element never hit those caches. sia-r65 does exactly this: for every tabbable target, it builds one Context.focus(target) and queries dozens of candidate elements' style under it. Interning collapses ~131.4s to ~0.4s on a 10k-node real-world page (sia-r62 and sia-r87, which have a similar shape, also improve substantially). Verified identical pass/fail/cantTell counts before and after on both the small and large local fixtures - this is a caching fix, not a change in what gets matched.
🦋 Changeset detectedLatest commit: eb80f6f The changes in this PR will be included in the next version bump. This PR includes changesets to release 76 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
🟡 Changes recommended
The new identity and isolation guarantees lack direct regression tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Interns single-state Context factories to improve downstream cache reuse and performance.
Changes:
- Adds per-state, per-element context caches.
- Documents behavior and measured performance gains.
File summaries
| File | Description |
|---|---|
packages/alfa-selector/src/context.ts |
Implements context interning. |
.changeset/context-interning.md |
Records the patch and impact. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| */ | ||
| public static hover(element: Element): Context { | ||
| return this.empty().hover(element); | ||
| return this._hovered.get(element, () => this.empty().hover(element)); |
|
@lukasbob thanks a lot for this work, Mads! |
single-audit speedup Traced the actual mechanism with instrumented call counters: a single, fresh evaluation of sia-r65 gets zero benefit (100% Cascade.get() cache miss with or without interning). The ~131s -> ~0.4s figure only happens on a *second* evaluation of the *same* already-loaded page, because the cache is scoped to the Document/device, not to one rule run. Since production evaluates every rule exactly once per page, this fix has no measurable real-world performance impact - it remains correct and worth keeping (removes a real cache-identity bug), just not for the reason originally stated.
You are very right to question this. I made a mistake here, by running two passes over the rules, warming the cache in the first run. This is not how the checks are run in our pipeline, so the real impact is likely negligible. Apologies! |
Thanks a lot for the details, and please don't apologize for this great work. I learned about the performance bottlenecks here and I really appreciate it :-) |
Summary
Context.focus(element)(and the other single-state static factories —.hover(),.active(),.visit()) constructed a brand-newContextinstance on every call, even for the same element.alfa-cascade/alfa-style's internal caches areWeakMap-keyed onContextobject identity, and those caches are scoped to theDocument/device (they persist for the life of the page object, not just one rule's evaluation) — so repeated queries for the same element under the same single-state context missed the cache every time, even across two full evaluations of the same already-loaded page.This interns the four factories (
Cache<Element, Context>per state, returning the same instance for the same element). Pure caching fix — no change to which selectors match or whatContext.getState/.hasState/etc. return for any given element.Correction: this has ~no measurable production impact
The PR originally claimed a large real-world performance win based on
alfa-benchmark's comparison numbers (sia-r65-99.67% etc.). That framing was wrong. Traced the actual mechanism with instrumented call counters onContext.focus()andCascade.get()'s non-empty-context path:sia-r65run once, cold, against a fresh page: 100% ofCascade.get()calls miss (50,278/50,278) — identical with or without this fix, ~135-140s either way.sia-r65run a second time against the same already-loaded page (which is exactly whatalfa-benchmark'srun.tsdoes: one discarded warmup pass, then the timed pass the benchmark numbers come from): with the fix, the second pass collapses to ~0.5s. Without the fix, the second pass is just as slow as the first.The fix only pays off when the same
Document/device is evaluated more than once in the same process — which is exactly the benchmark harness's warmup+timed methodology, but is not how production auditing works: every rule is evaluated exactly once per page, so there's no second pass to benefit from. This fix is confirmed to have effectively zero real-world performance impact, despite the dramatic benchmark numbers.It's kept anyway as a correctness-neutral cache-identity fix (harmless, verified safe, technically sound), not as a performance fix. The original
sia-r65/sia-r62production performance problem that motivated this investigation remains unsolved.Test plan
yarn build packages/alfa-selector(+ downstream:alfa-cascade,alfa-style,alfa-rules,alfa-act)yarn test packages/alfa-selector packages/alfa-cascade packages/alfa-style packages/alfa-rules packages/alfa-act— 2146/2146 passingsia-r65against two local fixtures — identical pass/fail/cantTell counts with and without the fixyarn changeset status— clean🤖 Generated with Claude Code