Skip to content

Perf: Eliminate double normalization of params in callParts#21318

Merged
NullVoxPopuli merged 1 commit intoemberjs:mainfrom
johanrd:perf/normalize-dedupe
Apr 17, 2026
Merged

Perf: Eliminate double normalization of params in callParts#21318
NullVoxPopuli merged 1 commit intoemberjs:mainfrom
johanrd:perf/normalize-dedupe

Conversation

@johanrd
Copy link
Copy Markdown
Contributor

@johanrd johanrd commented Apr 17, 2026

Eliminates redundant work in packages/@glimmer/syntax/lib/v2/normalize.ts. Parser-independent — works with current Jison pipeline.

The fix

callParts() was normalizing all parameters twice — once to calculate paramLoc via SpanList.range(), then again to build the positional args. The paramList from the first pass is already ASTv2.ExpressionNode[] and can be passed directly to builder.positional().

The duplication dates back to the original 2020 commit where paramList was computed at line 228 but not reused at line 234, with an identical .map(normalize) call created instead.

Why it's safe: symbol allocations (allocateFree/allocateNamed) are idempotent (indexOf/cache check before allocating), PositionalArguments stores exprs as readonly, and SpanList.range() only reads .loc from the nodes.

Where the savings come from: callParts() is called once per helper/component invocation that has params — {{helper a b}}, {{#if cond}}, (subexpr a), etc. Plain path expressions like {{this.name}} don't go through callParts and are unaffected. The benchmark fixture has 12 param-bearing invocations per ~1.5k chars (36% of all mustaches), which is representative of a typical route template with moderate interactivity. Templates with higher helper/sub-expression density will see larger gains.

Benchmark (pnpm bench:precompile, averaged over 5 paired runs)

Apple M1 Max, Node 24.14.

Prod build

size main this PR speedup
small (1517c) 1.80 ms 1.76 ms 2%
medium (4551c) 5.54 ms 5.40 ms 3%
large (33374c) 44.29 ms 43.05 ms 3%

Dev build

size main this PR speedup
small (1517c) 1.89 ms 1.78 ms 6%
medium (4551c) 5.57 ms 5.31 ms 5%
large (33374c) 45.69 ms 42.26 ms 8%

Dev build shows a larger improvement because the redundant normalization also triggers DEBUG assertions (e.g. the charPosFor round-trip check) that are eliminated along with the duplicate pass.

Reproduce: pnpm build && pnpm bench:precompile

callParts() was normalizing all parameters twice — once to calculate
paramLoc via SpanList.range(), then again to build the positional args.

The paramList computed on the first pass is already ASTv2.ExpressionNode[]
and can be passed directly to builder.positional(). Safe because:

- Symbol allocations (allocateFree/allocateNamed) are idempotent — they
  check indexOf/cache before allocating, so the second pass was pure waste.
- PositionalArguments stores exprs as readonly — no mutation concern.
- SpanList.range() only reads .loc from the nodes — no stored references.

The duplication dates back to the original 2020 commit (8e11b91) where
paramList was computed for SpanList.range() but then not reused for the
builder, with an identical .map(normalize) call created instead.

Impact (pnpm bench:precompile, prod dist, Apple M1 Max):
- normalize large (33374c): 18.92ms → 17.72ms (6%)
- precompile large: 43.01ms → 41.12ms (4%)
@johanrd johanrd marked this pull request as ready for review April 17, 2026 08:05
@johanrd johanrd changed the title [CLEANUP] Eliminate double normalization of params in callParts [CLEANUP] Perf: Eliminate double normalization of params in callParts Apr 17, 2026
@johanrd johanrd changed the title [CLEANUP] Perf: Eliminate double normalization of params in callParts Perf: Eliminate double normalization of params in callParts Apr 17, 2026
Copy link
Copy Markdown
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tyty

@NullVoxPopuli NullVoxPopuli merged commit 96e8cca into emberjs:main Apr 17, 2026
43 checks passed
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