-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(hir): keep observable user code out of a folded builder (#10357) #10361
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
proggeramlug
wants to merge
2
commits into
PerryTS:main
from
proggeramlug:fix-10357-fold-toprimitive
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,37 @@ | ||
| Fixed a 75× property-store cliff on `const o = {}; const X = 1; o.a = X;` | ||
| (#10353). The straight-line builder fold (#6812) rewrites `const o = {}` | ||
| plus its following `o.k = v` assignments into the object literal they spell | ||
| out, which is what gives the object a closed anon shape, a shape-stamped | ||
| allocation and direct stores. It only matched when the assignments followed | ||
| the binding *immediately*, so a single ordinary declaration in between — the | ||
| usual way initialisation code names its constants — dropped the whole | ||
| sequence back onto the dynamic `js_put_value_set` path, where every store | ||
| re-interns and re-coerces the key and transitions the object's shape. The | ||
| same program with the value passed as a parameter, or with the constants | ||
| written inline, was 75× faster, which is what made the cliff look like a | ||
| property of the stored *value*. | ||
|
|
||
| `fold_builder_sequences` now skips up to 64 statements between an **empty** | ||
| `{}` binding and its first assignment, sinking the allocation below them. A | ||
| statement is skippable only when moving the allocation past it is | ||
| unobservable, which is the pair of conditions the value side already carries | ||
| (`gap_stmt_is_hoistable`): it must not name the binding, and it must not be | ||
| able to execute user code — a call can reach a hoisted | ||
| `function peek() { return o; }` that names the binding without the statement | ||
| naming it, which would turn a successful read into a TDZ `ReferenceError`. | ||
| Destructuring patterns (getter-bearing property reads) and populated | ||
| literals are excluded; sinking `const o = { a: y }` below `const y = 1` | ||
| would hide a TDZ throw. Skipped statements keep their relative order and | ||
| still run before every folded value. | ||
|
|
||
| Measured with `perf stat -e instructions:u` on x86_64, 2400 iterations | ||
| building a six-property object with `--no-auto-optimize`: 108,447,339 → | ||
| 1,399,772 instructions (77×), matching the same program with the constants | ||
| written inline (1,401,872) or the value passed as a parameter (1,411,774). | ||
| Nothing that folded before folds differently — the gap is an additional | ||
| match, and a statement that fails the test leaves the original dynamic | ||
| writes exactly as they were: `benchmarks/object-write-6812` and the | ||
| `bench_*` corpus move by at most 0.006%, and a 12k-line file whose gaps | ||
| never reach an assignment (maximum pre-scan work, zero folds) costs 0.019% | ||
| more to compile. A file where the fold now applies compiles 51% cheaper, | ||
| because 1,200 dynamic store sites become 200 stamped allocations. | ||
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,44 @@ | ||
| Fixed the builder fold (#6812) turning a successful read into a TDZ | ||
| `ReferenceError` when a folded value runs an implicit conversion (#10357). | ||
| Folding `const o = {}; o.a = v;` into `const o = { a: v }` evaluates `v` before | ||
| `o` is initialized, which is unobservable only if `v` runs no user code that | ||
| can read `o`. `value_is_fold_safe` claimed exactly that, yet admitted every | ||
| converting operator: `"" + w`, `-w`, `w < 1`, `w == 1` and `` `${w}` `` all call | ||
| `w`'s `valueOf`/`toString`/`Symbol.toPrimitive`. With | ||
| `w = { valueOf() { return o; } }` node builds `{ a: "[object Object]" }` and | ||
| perry threw. | ||
|
|
||
| Every such conversion could have been dropped from the predicate, but that | ||
| would have stopped the fold's own motivating example (`o.b = r + i`) from | ||
| folding. Instead the fold now asks whether anything *can* read the binding | ||
| early. A binding is only readable by code that names it, so user code reached | ||
| through a conversion must be a function-like nested in the builder's scope | ||
| that mentions the name. `FoldScope` scans each function body (or the module) | ||
| for such observers, and keeps that scan precise because every false positive | ||
| costs a fold: | ||
|
|
||
| - a function-like created after a `let`/`const` builder cannot run before it | ||
| (the binding is fresh per loop pass and execution within a pass only moves | ||
| forward); a hoisted function declaration always counts, and so does any | ||
| observer of a `var`, whose binding every loop pass shares; | ||
| - a nested function-like that re-binds the name as a parameter or top-level | ||
| body declaration is not an observer (body declarations do not shadow | ||
| parameter defaults); | ||
| - `eval` (Perry compiles a literal `eval("o")` into a closure that reads | ||
| `o`), `with`, an exported name and a module-level `var` make a builder | ||
| observable outright. | ||
|
|
||
| The same hazard hides in a bare identifier read: a name that resolves to no | ||
| binding reads the global object, and `Object.defineProperty(globalThis, "g", | ||
| { get() { return o; } })` makes that read user code. `o.a = g` threw exactly | ||
| like the conversion. An unobservable builder folds exactly as before. On an | ||
| observable builder, a conversion needs operands that are primitive by | ||
| construction, and a read needs a proven declarative binding. `Visible` | ||
| proves that from a chain built only of declarations that cover the whole | ||
| region: a statement list's own declarations, parameters and | ||
| function-scoped `var`s, loop heads, catch parameters, and module imports and | ||
| declarations. A sibling block's declaration, an ambient `declare` (see | ||
| #10363), or any `with` in the module does not count. #10355's gap statements | ||
| share the same answers. The scan covers the enclosing function rather than | ||
| the folded statement list, because a `var` is function-scoped and a `let` in | ||
| one `case` is visible to every other case of its `switch`. |
Oops, something went wrong.
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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the "nothing folds differently" claim, or scope it to the conversion rule.
This PR also ships
changelog.d/10361-builder-fold-toprimitive.md, which stops folding converting values ("" + w,-w,`${w}`) for builders that code in scope can read early. Those values folded before this release. When the two fragments are assembled into one release note, this sentence contradicts that entry.State the gap rule as an additional match and let the conversion fragment own the behavior change, or say explicitly that the only fold that is withdrawn is the observable conversion case.
📝 Proposed wording change
Based on learnings, changelog fragments in
changelog.d/must describe the final shipped behavior as one coherent release-note entry and must not carry development-slice narratives that contradict one another once the release notes are assembled.🤖 Prompt for AI Agents
Source: Learnings