-
-
Notifications
You must be signed in to change notification settings - Fork 161
perf(codegen): elide wasted template-literal concat-chain parts (−18.6%) #10576
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| ### Performance | ||
|
|
||
| - Cut the instruction count of a template literal that opens on a | ||
| substitution (`` `${x}...` ``, the common shape — no literal text before | ||
| the first `${`) by eliding two sources of wasted work in its desugared | ||
| `js_string_concat_chain` call: | ||
| - The leading quasi is skipped when empty instead of unconditionally | ||
| seeding the chain with a literal `Expr::String("")`. Every *interior* | ||
| quasi already had this guard; the leading one never did, so a template | ||
| opening on `${` always carried one extra, always-empty part through | ||
| classification, and a single-substitution template (`` `${x}` ``) missed | ||
| the concat-chain fold's 3-part minimum entirely, falling back to the | ||
| pairwise path to concatenate an empty string for nothing. | ||
| - A `number`-typed parameter's substitution now drops its redundant | ||
| `StringCoerce` wrapper even when codegen has no dataflow *proof* it is | ||
| numeric, only the declared annotation — mirroring how | ||
| `is_declared_string_expr` already trusts a declared `string` a few call | ||
| sites up the stack. This is sound because `js_string_concat_chain`'s own | ||
| part classifier tag-dispatches every part itself, and for any shape that | ||
| isn't a plain number it falls back to the exact `js_jsvalue_to_string` / | ||
| `js_string_materialize_to_heap` calls `js_string_coerce` forwards to for | ||
| those same shapes — so a lying `number` annotation still produces | ||
| byte-identical output, and only a genuine number additionally skips a | ||
| throwaway intermediate heap string. | ||
|
|
||
| Measured on `` `${s}:${n}` `` (`s` a short string, `n` a non-integer | ||
| double), differencing two probes to cancel fixed per-process cost (median | ||
| of 7, N=20000; `loop16`/`loop80` control read ~0 in both arms): **1173 → | ||
| 955 instructions per evaluation (−18.6%)**. An integer-interpolation | ||
| variant (`` `${s}:${i}` ``, `i` a proven loop counter that already had the | ||
| numeric fast path) isolates the leading-quasi fix alone: 713 → 675 | ||
| (−5.4%), confirming the larger non-integer win comes from the | ||
| `StringCoerce` elision. |
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| // A template literal that OPENS on a substitution (`` `${x}...` ``, the | ||
| // overwhelmingly common shape: no literal text before the first `${`) used | ||
| // to unconditionally seed its desugared concat chain with a real, always- | ||
| // empty `Expr::String("")` part — the leading quasi was the only one that | ||
| // never got the "skip when empty" guard interior/trailing quasis already | ||
| // had. That wasted classification slot survived all the way to | ||
| // `js_string_concat_chain`, and for a single-substitution template | ||
| // (`` `${x}` ``) it also defeated the >=3-part minimum for the n-way | ||
| // concat-chain fold entirely, forcing the pairwise path to concatenate a | ||
| // literal "" for nothing. | ||
| // | ||
| // Separately, a `number`-typed parameter's template substitution used to | ||
| // keep its `StringCoerce` wrapper (materializing an intermediate heap | ||
| // string via `js_string_coerce` before the chain call ever saw it) because | ||
| // only a *proven* (not merely declared) non-pointer local elided it — and a | ||
| // plain public-body function parameter is never proof-bearing, only | ||
| // declaration-bearing. Both fixes are covered together here because they | ||
| // compound: the leading substitution is exactly where the elided part | ||
| // becomes the flattened chain's FIRST entry rather than an interior one. | ||
|
|
||
| function tpl(s: string, n: number): string { | ||
| return `${s}:${n}`; | ||
| } | ||
|
|
||
| // The leading-substitution shapes the elision above targets. | ||
| function lead1(n: number): string { | ||
| return `${n}`; | ||
| } | ||
| function lead2(n: number, s: string): string { | ||
| return `${n}:${s}`; | ||
| } | ||
| function leadInt(i: number): string { | ||
| return `${i}!`; | ||
| } | ||
|
|
||
| // Multi-part chain (>=3 substitutions), still opening on `${`. | ||
| function multi(a: number, b: string, c: number, d: string): string { | ||
| return `${a}-${b}-${c}-${d}`; | ||
| } | ||
|
|
||
| // Integer vs non-integer interpolation. | ||
| console.log(tpl("abc", 3), tpl("abc", 3.5), tpl("abc", -0), tpl("abc", NaN)); | ||
| console.log(leadInt(0), leadInt(-1), leadInt(1000000)); | ||
|
|
||
| // Leading substitution, single and multi-part. | ||
| console.log(lead1(42), lead1(1 / 3), lead1(-0), lead1(Infinity)); | ||
| console.log(lead2(7, "x"), lead2(2.5, ""), lead2(-9, "tail")); | ||
|
|
||
| // Empty string operands on both sides of the elided number. | ||
| console.log(tpl("", 5), tpl("", 5.25), lead2(0, "")); | ||
|
|
||
| // Multi-part chain. | ||
| console.log(multi(1, "a", 2, "b"), multi(-1.5, "", 0, "z")); | ||
|
|
||
| // Result short enough for SSO (<=5 bytes total) alongside a longer one. | ||
| console.log(`${1}${2}`, `${"ab"}${12}`, `${lead1(9)}${"x"}${9}`); | ||
|
|
||
| // Non-ASCII and surrogate-pair content flowing through the leading part and | ||
| // through an interior number part. | ||
| const emoji = String.fromCharCode(0xd83d) + String.fromCharCode(0xde00); | ||
| console.log(`${emoji}:${3}`, `${"héllo"}:${7.5}`, `${3}:${emoji}`); | ||
| // Adjacent lone surrogates split across TWO parts must still canonicalize | ||
| // into one astral scalar in the chained result. | ||
| const hi = String.fromCharCode(0xd83d); | ||
| const lo = String.fromCharCode(0xde00); | ||
| console.log(`${hi}${lo}:${1}`, (`${hi}${lo}:${1}`).length); | ||
|
|
||
| // A lying `number` annotation reaching the LEADING (now-unwrapped) position: | ||
| // the elision must fall back to the exact same coercion `String(x)` uses, | ||
| // not misread the bits. | ||
| console.log(lead1("nine" as any), lead1(true as any), lead1(null as any)); | ||
| console.log(lead1({ toString: () => "OBJ" } as any)); | ||
| const both = { | ||
| valueOf() { | ||
| return 111; | ||
| }, | ||
| toString() { | ||
| return "STR"; | ||
| }, | ||
| }; | ||
| console.log(lead1(both as any), lead2(both as any, "s")); | ||
|
|
||
| // A substitution whose (elided) coercion still must not double-evaluate or | ||
| // reorder relative to its neighbors. | ||
| let calls = 0; | ||
| function counted(): any { | ||
| calls++; | ||
| return 7; | ||
| } | ||
| console.log(`${counted()}:${counted()}`, calls); | ||
|
|
||
| // Hot loop shape: repeated leading-substitution template, integer and | ||
| // fractional, accumulated. | ||
| let acc = ""; | ||
| for (let i = 0; i < 50; i++) acc = `${i}:${i / 3}`; | ||
| console.log(acc, acc.length); |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 16672
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 15672
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 9124
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 9295
Keep
StringCoercefor declaration-only number locals.lower_string_concat_chainlowers every substitution before callingjs_string_concat_chain. If a declarednumberlocal contains an object, the runtime helper performsjs_jsvalue_to_stringonly after the later substitution has run. This can change observable conversion order and output.Restrict this elision to values proven numeric by runtime or dataflow analysis. The current tests cover lying values and conversion separately, but not this ordering case.
Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents