Inline GetCombinedNodeFlags and GetCombinedModifierFlags bodies#4240
Inline GetCombinedNodeFlags and GetCombinedModifierFlags bodies#4240mds-ant wants to merge 1 commit into
GetCombinedNodeFlags and GetCombinedModifierFlags bodies#4240Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds a benchmark for combined flag calculation and refactors GetCombined*Flags helpers to inline their logic (likely to reduce overhead and improve performance).
Changes:
- Added
BenchmarkGetCombinedFlagsto measureGetCombinedNodeFlags/GetCombinedModifierFlagsover real fixture ASTs. - Inlined combined-flag traversal logic into
GetCombinedModifierFlagsandGetCombinedNodeFlags, removing the generic helper and function-pointer indirection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/ast/utilities_bench_test.go | Adds a benchmark that parses fixture files, collects declarations, and measures combined flag computations. |
| internal/ast/utilities.go | Refactors combined flag computation by inlining logic for node vs modifier flags. |
|
This is a microoptimization; do you have anything that says this is hot code? The old times are already microseconds. |
|
This came up during a performance audit of the codebase. I agree the win is small, but I think it might still be worth landing given that (1) the relative win is measurable and (2) we're not introducing additional complexity or a maintenance burden. |
|
Hm, but as the copilot comment notes, this involves a copy paste |
|
The duplication is what it takes to unlock this optimization though. Any shared helper either reintroduces indirect calls or grows But as you said, this is a microoptimization. If you'd rather not have this duplication (however small), we can close this PR. |
Replace the generic getCombinedFlags[T] helper with direct field reads in GetCombinedNodeFlags (node.Flags) and GetCombinedModifierFlags (node.ModifierFlags()). The parent-walk and OR-accumulation are unchanged; this just removes the indirect call through a function pointer at each step. getCombinedFlags and the trivial getNodeFlags wrapper had no other callers and are removed. Adds BenchmarkGetCombinedFlags over the standard fixture set. BenchmarkGetCombinedFlags (interleaved n=12, GOGC=off, benchtime=200x): | Fixture | Before | After | Δ | |---------------------------|---------------|--------------|---------| | checker.ts | 805.4 µs | 610.8 µs | −24.2% | | dom.generated.d.ts | 284.0 µs | 177.9 µs | −37.4% | | Herebyfile.mjs | 11.22 µs | 6.17 µs | −45.0% | | jsxComplexSignature….tsx | 2.73 µs | 1.53 µs | −43.8% | VS Code end-to-end (tsgo -p vscode/src --noEmit), single-threaded: | Metric | Before | After | Δ | |---------------------------------------|------------------|-----------------|-------------------------| | instructions:u (GOGC=off, min-of-3) | 100,168,751,028 | 99,897,715,729 | −0.27% | | wall, mean (interleaved n=14) | 20.92 s | 20.66 s | −1.24% (p=0.149, n.s.) | | Types / Symbols / Instantiations | identical | identical | | VS Code end-to-end, parallel (default GOMAXPROCS): | Metric | Before | After | Δ | |---------------------------------|---------|---------|-------------------------| | wall, mean (interleaved n=16) | 7.07 s | 7.12 s | +0.77% (p=0.652, n.s.) |
a48ada2 to
32ac1f1
Compare
This PR replaces the generic
getCombinedFlags[T]helper with direct field reads inGetCombinedNodeFlags(node.Flags) andGetCombinedModifierFlags(node.ModifierFlags()). The parent-walk and OR-accumulation are unchanged; this just removes the indirect call through a function pointer at each step.getCombinedFlagsand the trivialgetNodeFlagswrapper had no other callers and have been removed. AddsBenchmarkGetCombinedFlagsover the standard fixture set.This PR was assisted by Claude Code.
Performance