Skip to content

Inline GetCombinedNodeFlags and GetCombinedModifierFlags bodies#4240

Open
mds-ant wants to merge 1 commit into
microsoft:mainfrom
mds-ant:perf/combined-flags-direct
Open

Inline GetCombinedNodeFlags and GetCombinedModifierFlags bodies#4240
mds-ant wants to merge 1 commit into
microsoft:mainfrom
mds-ant:perf/combined-flags-direct

Conversation

@mds-ant

@mds-ant mds-ant commented Jun 8, 2026

Copy link
Copy Markdown

This PR replaces 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 have been removed. Adds BenchmarkGetCombinedFlags over the standard fixture set.

This PR was assisted by Claude Code.

Performance

goos: darwin
goarch: arm64
pkg: github.com/microsoft/typescript-go/internal/ast
cpu: Apple M1 Pro
                                                                 │ /tmp/baseline.txt │           /tmp/patched.txt           │
                                                                 │      sec/op       │    sec/op     vs base                │
GetCombinedFlags/empty.ts-10                                            1.460n ± 14%   1.458n ± 15%        ~ (p=0.827 n=30)
GetCombinedFlags/checker.ts-10                                          666.9µ ±  0%   504.2µ ±  0%  -24.40% (p=0.000 n=30)
GetCombinedFlags/dom.generated.d.ts-10                                  246.0µ ±  1%   171.8µ ±  0%  -30.16% (p=0.000 n=30)
GetCombinedFlags/Herebyfile.mjs-10                                     11.435µ ±  0%   7.089µ ±  1%  -38.00% (p=0.000 n=30)
GetCombinedFlags/jsxComplexSignatureHasApplicabilityError.tsx-10        2.927µ ±  0%   1.913µ ±  1%  -34.61% (p=0.000 n=30)
geomean                                                                 6.037µ         4.434µ        -26.56%

Copilot AI review requested due to automatic review settings June 8, 2026 10:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 BenchmarkGetCombinedFlags to measure GetCombinedNodeFlags / GetCombinedModifierFlags over real fixture ASTs.
  • Inlined combined-flag traversal logic into GetCombinedModifierFlags and GetCombinedNodeFlags, 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.

Comment thread internal/ast/utilities_bench_test.go Outdated
Comment thread internal/ast/utilities.go
@jakebailey

Copy link
Copy Markdown
Member

This is a microoptimization; do you have anything that says this is hot code? The old times are already microseconds.

@RyanCavanaugh RyanCavanaugh added the No linked issue This PR doesn't say what bug it fixes label Jun 8, 2026
@RyanCavanaugh RyanCavanaugh added this to the Possible Improvement milestone Jun 8, 2026
@mds-ant

mds-ant commented Jun 8, 2026

Copy link
Copy Markdown
Author

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.

@jakebailey

Copy link
Copy Markdown
Member

Hm, but as the copilot comment notes, this involves a copy paste

@mds-ant

mds-ant commented Jun 9, 2026

Copy link
Copy Markdown
Author

The duplication is what it takes to unlock this optimization though. Any shared helper either reintroduces indirect calls or grows GetCombinedNodeFlags past the point where its own callers (IsVarConst, IsVarLet etc.) inline. The two functions are sitting right next to each other, so if the traversal logic ever changes, it should be easy to spot that it should change in two places.

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.)  |
@mds-ant mds-ant force-pushed the perf/combined-flags-direct branch from a48ada2 to 32ac1f1 Compare June 9, 2026 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

No linked issue This PR doesn't say what bug it fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants