perf(vapor): reduce codegen size for single DOM bindings#14845
Draft
edison1105 wants to merge 8 commits into
Draft
perf(vapor): reduce codegen size for single DOM bindings#14845edison1105 wants to merge 8 commits into
edison1105 wants to merge 8 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughThis PR introduces reactive DOM binding effect helpers to Vue's Vapor compiler and runtime. It refactors the compiler to emit specialized ChangesDOM Binding Effects Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/compiler-vapor
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/runtime-vapor
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
Size ReportBundles
Usages
|
edison1105
force-pushed
the
edison/perf/singleOperationCodegen
branch
from
efd4800 to
0e9c26a
Compare
edison1105
changed the title
![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
🧹 Nitpick comments (1)
packages/runtime-vapor/__tests__/bench/domBinding.bench.ts (1)
247-258: ⚡ Quick winMake
setEventbenchmark paths allocation-equivalentThe old path creates a new invoker every reactive run (Line 248), while the binding path creates it once. That biases the comparison beyond codegen-wrapper cost.
Proposed fix
benchBinding( 'setEvent', createDiv, (el, source) => { + const invoker = createInvoker(noop) renderEffect(() => - on(el, source.value & 1 ? 'click' : 'mouseover', createInvoker(noop), { + on(el, source.value & 1 ? 'click' : 'mouseover', invoker, { effect: true, }), ) }, (el, source) => { + const invoker = createInvoker(noop) setEventBinding( el, () => (source.value & 1 ? 'click' : 'mouseover'), - createInvoker(noop), + invoker, ) }, )🤖 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 `@packages/runtime-vapor/__tests__/bench/domBinding.bench.ts` around lines 247 - 258, The benchmark currently allocates a new invoker inside the reactive run for the "old path" (renderEffect -> on(..., createInvoker(noop), ...)) while the "binding path" reuses a single invoker, biasing results; fix by creating the invoker once outside the reactive/run scope (use a local const inv = createInvoker(noop)) and pass that same inv to both renderEffect/on and setEventBinding calls (referencing renderEffect, on, setEventBinding, createInvoker, and noop) so both paths have allocation-equivalent behavior.
🤖 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 `@packages/runtime-vapor/src/dom/bindingEffect.ts`:
- Around line 418-424: toEventBindingOptions currently mutates the
caller-provided options by assigning effect = true to the passed-in object;
instead create and return a new EventBindingOptions object so callers' objects
are not modified. In the toEventBindingOptions function, copy the incoming
AddEventListenerOptions into a fresh object (e.g. via object spread or
Object.assign) and set effect: true on that new object, then return it; keep the
return type EventBindingOptions and handle the undefined case by returning an
object with only effect: true when no options were provided.
---
Nitpick comments:
In `@packages/runtime-vapor/__tests__/bench/domBinding.bench.ts`:
- Around line 247-258: The benchmark currently allocates a new invoker inside
the reactive run for the "old path" (renderEffect -> on(...,
createInvoker(noop), ...)) while the "binding path" reuses a single invoker,
biasing results; fix by creating the invoker once outside the reactive/run scope
(use a local const inv = createInvoker(noop)) and pass that same inv to both
renderEffect/on and setEventBinding calls (referencing renderEffect, on,
setEventBinding, createInvoker, and noop) so both paths have
allocation-equivalent behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f6b9d9e-cdd6-48e0-9359-6af5ea3e033e
⛔ Files ignored due to path filters (14)
packages/compiler-vapor/__tests__/__snapshots__/compile.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/__snapshots__/staticTemplate.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/expression.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/transformChildren.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/transformElement.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/transformKey.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vBind.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vFor.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vHtml.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vIf.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vModel.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vOn.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vSlot.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vText.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (19)
packages/compiler-vapor/__tests__/compile.spec.tspackages/compiler-vapor/__tests__/transforms/transformChildren.spec.tspackages/compiler-vapor/__tests__/transforms/transformElement.spec.tspackages/compiler-vapor/__tests__/transforms/vBind.spec.tspackages/compiler-vapor/__tests__/transforms/vHtml.spec.tspackages/compiler-vapor/__tests__/transforms/vOn.spec.tspackages/compiler-vapor/__tests__/transforms/vText.spec.tspackages/compiler-vapor/src/generators/block.tspackages/compiler-vapor/src/generators/event.tspackages/compiler-vapor/src/generators/expression.tspackages/compiler-vapor/src/generators/html.tspackages/compiler-vapor/src/generators/operation.tspackages/compiler-vapor/src/generators/prop.tspackages/compiler-vapor/src/generators/text.tspackages/runtime-vapor/__tests__/bench/domBinding.bench.tspackages/runtime-vapor/__tests__/bench/textBinding.bench.tspackages/runtime-vapor/__tests__/renderEffect.spec.tspackages/runtime-vapor/src/dom/bindingEffect.tspackages/runtime-vapor/src/index.ts
edison1105
force-pushed
the
edison/perf/singleOperationCodegen
branch
2 times, most recently
from
e8d5bbf to
4dd2c1a
Compare
Lower single-operation Vapor DOM updates to dedicated binding helpers, including text, props, html, class/style/value, and event bindings. The helpers reuse RenderEffect lifecycle and scheduling semantics while avoiding the extra renderEffect wrapper in generated code. Keep multi-operation effects and shared-expression cases on the existing renderEffect path to preserve batching and cached expression behavior. Add compiler snapshots, runtime coverage, and binding benchmarks for the new paths.
edison1105
force-pushed
the
edison/perf/singleOperationCodegen
branch
from
4dd2c1a to
b64fc28
Compare
edison1105
force-pushed
the
edison/perf/singleOperationCodegen
branch
from
7cf03b6 to
58bc177
Compare
…erationCodegen # Conflicts: # packages/compiler-vapor/__tests__/transforms/__snapshots__/vSlot.spec.ts.snap
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/runtime-vapor/src/dom/bindingEffect.ts (1)
506-511: ⚡ Quick winUse
setDynamicEventsin once-slot path for behavior parity and nullish safety.Line 507–510 manually iterates
getter(); this can throw on nullish values and may diverge fromsetDynamicEventssemantics. Reusing the shared helper keeps one behavior surface.Suggested fix
export function setDynamicEventsBinding( el: HTMLElement, getter: () => Record<string, (...args: any[]) => any>, ): void { if (inOnceSlot) { - const events = getter() - for (const name in events) { - on(el, name, events[name]) - } + setDynamicEvents(el, getter()) return }🤖 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 `@packages/runtime-vapor/src/dom/bindingEffect.ts` around lines 506 - 511, Replace the manual for-loop in the inOnceSlot branch with the shared helper setDynamicEvents to keep behavior parity and nullish-safety: instead of calling getter() and iterating with on(el, name, ...), call setDynamicEvents(el, getter() ?? null) (or pass the raw getter() if setDynamicEvents accepts null/undefined) and remove the for-loop and on calls so the once-slot path uses the same event-install semantics as the non-once path.
🤖 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.
Nitpick comments:
In `@packages/runtime-vapor/src/dom/bindingEffect.ts`:
- Around line 506-511: Replace the manual for-loop in the inOnceSlot branch with
the shared helper setDynamicEvents to keep behavior parity and nullish-safety:
instead of calling getter() and iterating with on(el, name, ...), call
setDynamicEvents(el, getter() ?? null) (or pass the raw getter() if
setDynamicEvents accepts null/undefined) and remove the for-loop and on calls so
the once-slot path uses the same event-install semantics as the non-once path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: abc19f9a-92cd-4cf2-87f7-b47331d9653d
⛔ Files ignored due to path filters (16)
packages/compiler-vapor/__tests__/__snapshots__/compile.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/__snapshots__/staticTemplate.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/expression.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/logicalIndex.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/transformChildren.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/transformElement.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/transformKey.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vBind.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vFor.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vHtml.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vIf.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vModel.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vOn.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vOnce.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vSlot.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vText.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
packages/compiler-vapor/__tests__/compile.spec.tspackages/compiler-vapor/__tests__/transforms/transformChildren.spec.tspackages/compiler-vapor/__tests__/transforms/transformElement.spec.tspackages/compiler-vapor/__tests__/transforms/vBind.spec.tspackages/compiler-vapor/__tests__/transforms/vHtml.spec.tspackages/compiler-vapor/__tests__/transforms/vOn.spec.tspackages/compiler-vapor/__tests__/transforms/vOnce.spec.tspackages/compiler-vapor/__tests__/transforms/vText.spec.tspackages/compiler-vapor/src/generators/block.tspackages/compiler-vapor/src/generators/event.tspackages/compiler-vapor/src/generators/expression.tspackages/compiler-vapor/src/generators/html.tspackages/compiler-vapor/src/generators/operation.tspackages/compiler-vapor/src/generators/prop.tspackages/compiler-vapor/src/generators/text.tspackages/runtime-vapor/__tests__/bench/domBinding.bench.tspackages/runtime-vapor/__tests__/bench/textBinding.bench.tspackages/runtime-vapor/__tests__/componentSlots.spec.tspackages/runtime-vapor/__tests__/renderEffect.spec.tspackages/runtime-vapor/src/dom/bindingEffect.tspackages/runtime-vapor/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/compiler-vapor/tests/transforms/vText.spec.ts
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.
Lower single-operation Vapor DOM updates to dedicated binding helpers, including
text, props, html, class/style/value, and event bindings. The helpers reuse
RenderEffect lifecycle and scheduling semantics while avoiding the extra
renderEffect wrapper in generated code.
Keep multi-operation effects and shared-expression cases on the existing
renderEffect path to preserve batching and cached expression behavior. Add
compiler snapshots, runtime coverage, and binding benchmarks for the new paths.
Summary by CodeRabbit
New Features
Improvements
Tests