[docs] Add detailed documentation for hole check elision pipeline [v8/v8 : main]
Toon Verwaest (Gerrit)
Toon Verwaest voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Toon Verwaest (Gerrit)
Toon Verwaest voted and added 1 comment![]( "Open in Gerrit")
Votes added by Toon Verwaest
| Auto-Submit | +1 |
1 comment
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Message from Olivier Flückiger![]( "Open in Gerrit")
I have reviewed the documentation in this CL. Here are my comments:
1. **Broken Links in `hole-check-elision.md`**:
The links to the other steps use absolute file URLs pointing to a local home directory (e.g., `file:///usr/local/google/home/verwaest/...`). These should be changed to relative links so they work correctly in the Gerrit UI and for other users.
* `docs/parsing/hole-check-elision.md` lines 43-45 should use relative paths like `hole-check-elimination-parser.md`.
2. **Minor Spec Precision in `hole-check-elimination-parser.md`**:
Line 35 states: "Function declarations are hoisted to the top of their scope (per FunctionDeclarationInstantiation), while class declarations are not (BlockDeclarationInstantiation)."
Technically, if a class declaration is at the top level of a function, its binding is instantiated in `FunctionDeclarationInstantiation` (Step 35), not `BlockDeclarationInstantiation`. Both result in the same TDZ behavior (created but not initialized), but for strict accuracy, it might be worth noting or just saying "lexically scoped declarations".
Overall, the documentation is excellent and very detailed!
Related details
- Leszek Swirski
- Olivier Flückiger
- Toon Verwaest
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Toon Verwaest (Gerrit)
Toon Verwaest voted and added 1 comment![]( "Open in Gerrit")
Votes added by Toon Verwaest
| Auto-Submit | +1 |
1 comment
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Toon Verwaest (Gerrit)
Toon Verwaest voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Olivier Flückiger (Gerrit)
Message from Olivier Flückiger![]( "Open in Gerrit")
The documentation provides an excellent and deep overview of V8's hole check elision pipeline. The structural progression from Parser -> Ignition -> Optimizing compilers is very clear.
I have reviewed the files and identified a few technical inaccuracies, mostly stemming from JS semantics in the examples and some nuanced compiler behaviors, that should be addressed before landing.
### `docs/parsing/hole-checks-bytecode.md`
1. **Example Code Inaccuracy (`let x, y;`)**: The examples consistently use `let x, y;` at the beginning of the functions. In JS, a declaration without an initializer implicitly initializes the variables to `undefined` immediately. Because of this, their `initializer_position` occurs before any accesses in the function body. The Parser (`src/ast/scopes.cc`) will statically evaluate `var->initializer_position() >= access_position` as FALSE, and mark the accesses as `HoleCheckMode::kElided`. The Bytecode Generator will NEVER see `kRequired` for these accesses, rendering the examples mechanically incorrect as illustrations of Ignition's dynamic elision. To fix this, restructure the examples to use cross-closure accesses to uninitialized variables (where the parser cannot statically prove initialization).
2. **Loop Execution Reasoning**: In the loops section, the document states: "V8 cannot guarantee that the loop body executes even once. Therefore, any hole checks performed inside the loop are conservatively forgotten." While true for `while`/`for`, this reasoning fails for `do...while` loops, which *are* guaranteed to execute at least once. However, V8 still conservatively forgets checks in `do...while` loops. The true technical reason is that V8's single-pass loop analysis (`VisitIterationBodyInHoleCheckElisionScope`) unconditionally wraps the body of *all* iteration statements in a `HoleCheckElisionScope`, which always discards its bitmap state upon destruction rather than merging it outward.
3. **Missing Second Strategy**: The document states "To maintain this bitmap correctly across control flow, V8 uses two primary strategies:" and introduces "1. Scoped State Preservation (`HoleCheckElisionScope`)", but the document abruptly ends. It is missing the second strategy: State Merging (via `HoleCheckElisionMergeScope`), which performs the set-intersection of the bitmaps at convergence points (crucial for understanding the `try/catch` and `switch` examples).
### `docs/parsing/hole-checks-optimized.md`
1. **Example Code Inaccuracy**: Similar to the previous file, the example in "Maglev's Implicit Control-Flow Elision" uses `let x;` which implicitly initializes the variable to `undefined`, bypassing the TDZ entirely.
2. **Turboshaft Lowering Context**: The Turboshaft section explains that hole checks are lowered into a standard comparison `RootEqual(value, RootIndex::kTheHoleValue)`. It would be helpful to explicitly mention that this lowering happens during the translation phase from Maglev IR to Turboshaft IR (`TurboLevGraphBuilder::Process(maglev::ThrowReferenceErrorIfHole...)`), as Maglev is now the frontend for Turboshaft.
3. **"Constant" Tracking Terminology**: In the section "Constant Tracking via Hole Elision and `maybe_assigned`", the document states that if a variable is not `maybe_assigned` and passes a hole check, Maglev will "specialize the load to a constant in the graph". This is slightly misleading. Maglev uses `known_node_aspects().GetContextCachedValue()` to cache and reuse the exact same `ValueNode` for all subsequent loads. While this eliminates redundant `LoadContextSlot` operations and their associated hole checks, the reused node is not necessarily a compiler *constant* (unless the original assignment was a constant); it is simply a reused graph node. Changing "constant" to "cached node" or "reused graph node" would be more technically accurate.
Related details
- Leszek Swirski
- Olivier Flückiger
- Toon Verwaest
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 9 comments![]( "Open in Gerrit")
To fix this, when V8 encounters a variable access inside a nested closure, it resolves the reference by recursively walking the scope chain outward, updating an `access_position` along the way. This adjusted position projects the inner access onto the boundary of the outer scope.Something is missing here. What do you mean exactly by "to fix this". So Far we have not seen a problem statement. You have introduced that nested closures pose challenges. I guess you are referring back to the "naive" resolution algorithm from "the baseline" breaking down? A bit of glue text is needed here and maybe a concrete example of what goes wrong.
This leads to a fascinating nuance: `inner` is a function expression, so it is *not* lexically hoisted within its own outer scope (`hoisted`). However, because `hoisted`'s scope is elided, the runtime context chain connects `inner` directly to `outer`.```suggestion
Since `inner` is a function expression it is *not* lexically hoisted within its own outer scope (`hoisted`). Still, because `hoisted`'s scope is elided, the runtime context chain connects `inner` directly to `outer`.
```
When a scope walk is short-circuited by a `cache_scope` hit, the recursive traversal is skipped. As a direct consequence, the `access_position` that gets passed down to the hole-check logic is "stale" — it has **not** been adjusted by the intermediate scopes because they were skipped.This part I don't understand. Why exactly is it stale. Where does the cache come from? From an earlier compile? This whole explanation went too fast.
## The Mechanics of `eval` ScopesThis part should go to the beginning of the section.
console.log(x); // Safe only if cond was true!hmm, but that is not the same as the above. x is just `undefined` here if the branch is not taken. this is not a TDZ. It does not throw.
Maybe you meant something like this:
```
function testTDZ(cond) {
switch(cond) {
case true:
let x = 1;
return x;
case false:
return x; // Throws ReferenceError: Cannot access 'x' before initialization
}
}
```
While the Parser (Step 1) performs a structural analysis to determine if a variable *ever* needs a hole check, the Bytecode Generator performs a **local, flow-sensitive analysis** to avoid emitting redundant check bytecodes within the same function.```suggestion
While the [Parser](hole-check-elimination-parser.md) performs a structural analysis to determine if a variable *ever* needs a hole check, the Bytecode Generator performs a **local, flow-sensitive analysis** to avoid emitting redundant check bytecodes within the same function.
```
console.log(x); // 1. Check 'x' required.As mentioned elsewhere, this `x` is initialized to `undefined`. No hole check required here.
let x;again, this `x` is initialized here.
### 3. `switch` Statements===========> Reviewed up to here. I think we need to clarify that we are talking about the same thing before I continue.
Related details
- Leszek Swirski
- Toon Verwaest
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Toon Verwaest (Gerrit)
Toon Verwaest added 11 comments![]( "Open in Gerrit")
Thanks for the review! I hope this is more clear. Too much pseudocode :)
let x = 10; // initializer_position = Pos 65This `let x = 10` should be moved under `MyClass` and we should smply leave a comment that `// essentially let x = hole at this point` That comment should end up above inner()
To fix this, when V8 encounters a variable access inside a nested closure, it resolves the reference by recursively walking the scope chain outward, updating an `access_position` along the way. This adjusted position projects the inner access onto the boundary of the outer scope.Something is missing here. What do you mean exactly by "to fix this". So Far we have not seen a problem statement. You have introduced that nested closures pose challenges. I guess you are referring back to the "naive" resolution algorithm from "the baseline" breaking down? A bit of glue text is needed here and maybe a concrete example of what goes wrong.
I guess it should be "To implement variable resolution of `x` without unnecessarily introducing hole checks.
This leads to a fascinating nuance: `inner` is a function expression, so it is *not* lexically hoisted within its own outer scope (`hoisted`). However, because `hoisted`'s scope is elided, the runtime context chain connects `inner` directly to `outer`.```suggestion
Since `inner` is a function expression it is *not* lexically hoisted within its own outer scope (`hoisted`). Still, because `hoisted`'s scope is elided, the runtime context chain connects `inner` directly to `outer`.
```
Acknowledged
When a scope walk is short-circuited by a `cache_scope` hit, the recursive traversal is skipped. As a direct consequence, the `access_position` that gets passed down to the hole-check logic is "stale" — it has **not** been adjusted by the intermediate scopes because they were skipped.This part I don't understand. Why exactly is it stale. Where does the cache come from? From an earlier compile? This whole explanation went too fast.
The entry in the cache. The cache is explained right above. Scope resolution caches resolved variables in its scope-tree basically where the tree turns into a linear line of scope-info backed Scopes (just outside of the currently compiled function).
## The Mechanics of `eval` ScopesThis part should go to the beginning of the section.
Acknowledged
console.log(x); // Safe only if cond was true!hmm, but that is not the same as the above. x is just `undefined` here if the branch is not taken. this is not a TDZ. It does not throw.
Maybe you meant something like this:
```
function testTDZ(cond) {
switch(cond) {
case true:
let x = 1;
return x;
case false:
return x; // Throws ReferenceError: Cannot access 'x' before initialization
}
}
```
Same as above, what I meant is to put a `// let x = hole` at line 31 and a real `let x;` under console.log.
While the Parser (Step 1) performs a structural analysis to determine if a variable *ever* needs a hole check, the Bytecode Generator performs a **local, flow-sensitive analysis** to avoid emitting redundant check bytecodes within the same function.```suggestion
While the [Parser](hole-check-elimination-parser.md) performs a structural analysis to determine if a variable *ever* needs a hole check, the Bytecode Generator performs a **local, flow-sensitive analysis** to avoid emitting redundant check bytecodes within the same function.
```
Acknowledged
console.log(x); // 1. Check 'x' required.As mentioned elsewhere, this `x` is initialized to `undefined`. No hole check required here.
Same as above.
again, this `x` is initialized here.
Same as above.
### 3. `switch` Statements===========> Reviewed up to here. I think we need to clarify that we are talking about the same thing before I continue.
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |