perf(components): memoize tv slot invocations with simple args - #6737
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/utils/tv.spec.ts`:
- Around line 162-165: Rename the test case around build to clarify that it
verifies identical output for reordered keys, not memoization cache reuse; leave
the assertion and implementation unchanged.
🪄 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
Run ID: 3b50c4c8-f271-457c-b88c-ebc679c4ffdb
📒 Files selected for processing (2)
src/runtime/utils/tv.tstest/utils/tv.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/utils/tv.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/utils/tv.ts (2)
140-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not key only enumerable properties.
JSON.stringify({ class: 'hidden' })becomes{}, andObject.defineProperty({}, 'class', { enumerable: false, value: 'hidden' })has no enumerable keys, so memoKey can return the same cache entry for an empty-like object and a{ class: 'hidden' }object.Object.keysalso omits accessor-based entries while slot resolution can read/getter-dependent properties, so non-enumerable/accessor descriptors should reject memoization before caching.🤖 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 `@src/runtime/utils/tv.ts` around lines 140 - 149, The memoization key logic around isMemoizable currently inspects only enumerable keys, allowing non-enumerable and accessor properties to collide with empty objects. Replace the Object.keys-based inspection with own-property descriptor inspection, rejecting any non-enumerable or accessor-based property before JSON.stringify; preserve the existing undefined-value serialization semantics for eligible data properties.
181-195: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winEnforce the cache limit before inserting the new entry.
With
cache.size > 500, a cache that already has 500 entries will insert a 501st entry on the next miss. Move the stale-cache reset into the miss path and use>= 500beforecache.setso the cache stays bounded.Proposed fix
- } else if (cache.size > 500) { - // Pathological dynamic inputs (e.g. per-row generated classes): - // reset rather than grow unbounded. - cache.clear() } let result = cache.get(cacheKey) if (result === undefined) { + if (cache.size >= 500) { + cache.clear() + } result = slot(slotProps) as string cache.set(cacheKey, result) }🤖 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 `@src/runtime/utils/tv.ts` around lines 181 - 195, Update the cache handling around the result lookup in the memoized slot flow: move the cache reset into the result-miss path, and before inserting a new result in cache.set, clear the cache when its size is at least 500. Preserve existing cache reuse for hits and ensure the newly computed entry is inserted afterward.
🧹 Nitpick comments (1)
test/utils/tv.spec.ts (1)
139-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the repeated-call test observe cache reuse.
toBeon primitive strings only verifies equal output; this test also passes if the slot is recomputed on every call. Spy on the underlying resolver or add an invocation counter, then assert the second identical call does not invoke it again.🤖 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 `@test/utils/tv.spec.ts` around lines 139 - 145, Update the repeated-identical-args test around ui.base to observe cache reuse rather than only string equality: spy on or count invocations of the underlying resolver, call ui.base with the same arguments twice, and assert the resolver runs only once while preserving the existing output assertions.
🤖 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 `@src/runtime/utils/tv.ts`:
- Around line 125-126: Update isMemoizable to track arrays currently being
traversed and return false when an array repeats on the active recursion stack,
preventing cyclic arrays from recursing indefinitely. Remove each array from the
active set after traversal so shared but non-cyclic arrays remain memoizable,
while preserving the existing uncached path for rejected values.
---
Outside diff comments:
In `@src/runtime/utils/tv.ts`:
- Around line 140-149: The memoization key logic around isMemoizable currently
inspects only enumerable keys, allowing non-enumerable and accessor properties
to collide with empty objects. Replace the Object.keys-based inspection with
own-property descriptor inspection, rejecting any non-enumerable or
accessor-based property before JSON.stringify; preserve the existing
undefined-value serialization semantics for eligible data properties.
- Around line 181-195: Update the cache handling around the result lookup in the
memoized slot flow: move the cache reset into the result-miss path, and before
inserting a new result in cache.set, clear the cache when its size is at least
500. Preserve existing cache reuse for hits and ensure the newly computed entry
is inserted afterward.
---
Nitpick comments:
In `@test/utils/tv.spec.ts`:
- Around line 139-145: Update the repeated-identical-args test around ui.base to
observe cache reuse rather than only string equality: spy on or count
invocations of the underlying resolver, call ui.base with the same arguments
twice, and assert the resolver runs only once while preserving the existing
output assertions.
🪄 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: 2a942854-5516-473d-b47e-40497ed2b0a5
📒 Files selected for processing (2)
src/runtime/utils/tv.tstest/utils/tv.spec.ts
`tv` returns `undefined` for a slot whose chain resolves to nothing (two on `navigation-menu`), so it can't double as the miss sentinel. Those slots re-ran on every call: 1,875 -> 60,416 ops/s on a 100-call loop.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/utils/tv.spec.ts`:
- Around line 241-249: Update the “keeps each slot cache bounded” test to
observe getter-read counts rather than only validating rendered class strings.
Verify that accessing entry 599 does not recompute it, while accessing entry 0
after the cache reset recomputes it, preserving the existing bounded-cache
scenario and output assertions as needed.
🪄 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: 51b1b7ed-a435-4107-927f-a845e0bddce6
📒 Files selected for processing (2)
src/runtime/utils/tv.tstest/utils/tv.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/utils/tv.ts
Merging this PR will improve performance by ×2.9
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | td x100 (string class) |
3,906.3 µs | 239 µs | ×16 |
| ⚡ | UButton |
5.7 ms | 4.7 ms | +20.78% |
| ⚡ | td x100 (array class) |
2.5 ms | 2.1 ms | +18.69% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing perf/tv-slot-memo (ae572ca) with v4 (5afbd5c)
benjamincanac
changed the title
|
@codspeedbot explain why this is faster |
Here's what the profiles say, comparing head ( The whole tailwind-variants slot-resolution subtree disappears from the profile. On
In head, none of those frames appear at all. The only frames left are Why the component-level number is what it is. On That also explains the 28 untouched benchmarks: mount benchmarks pay each slot call on a cold cache, so there is nothing to amortize, and factory-build benchmarks ( Two caveats on reading the report:
Summary
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot. |
benjamincanac
changed the title
1 participant
🔗 Linked issue
Relates to #6293
❓ Type of change
📚 Description
Every render re-invokes the tv slot functions (
ui.base(),ui.label(),ui.td(), ...), re-running variant resolution and twMerge even when the arguments are identical to the previous call. tailwind-variants caches nothing across invocations.This memoizes slot invocations in the
tvwrapper when the arguments are fully serializable: primitives and arrays of primitives, on a plain object. Anything else, clsx-style object classes and the(defaults) => classesreplacer functions from #6562, bails to the existing uncached path, so those semantics are untouched (covered bytest/utils/tv.spec.ts).JSON.stringifyis used as the cache key, and its dropping ofundefinedvalues matches tv's own treatment of undefined variants. Non-finite numbers bail becauseNaNserializes tonullbut resolves differently. The cache lives on the invocation result, so anapp.config.uichange or a variant-prop recompute starts fresh, and each slot's cache is capped at 500 entries to guard against pathological dynamic inputs.Where it pays off is a slot called many times with repeating arguments. A 100-call
ui.td()loop goes from 5,197 to 52,684 ops/s, andui.link()onnavigation-menufrom 1,533 to 42,094 ops/s.At component level that translates to one scenario: a Table re-render (200 rows x 5 columns, new data identity) goes from 12.2 to 13.7 ops/s, about 12%. Button mount, Button re-render and Table mount are unchanged. A Button makes roughly six slot calls per render, which is noise against a render that costs hundreds of microseconds, so the memo has nothing to amortize there.
The cost, for completeness: when the arguments never repeat, building the key is pure overhead. A 100-call loop with all-distinct classes is about 4% slower, and a 600-call one that runs past the 500-entry cap about 8% slower. That is the shape a Table with per-cell
meta.class.tdhits.Output classes are byte-identical: the full suite passes with zero snapshot changes.
📝 Checklist