Skip to content

perf(strict-void-return): reuse function-body walk visitor closure#1033

Merged
camc314 merged 3 commits into
mainfrom
perf/strict-void-return
Jul 1, 2026
Merged

perf(strict-void-return): reuse function-body walk visitor closure#1033
camc314 merged 3 commits into
mainfrom
perf/strict-void-return

Conversation

@connorshea

Copy link
Copy Markdown
Member

Summary

strict-void-return walks each function body (visit) looking for non-void
return statements. The ForEachChild callback was declared inline inside
the recursive visit
, so Go heap-allocated a fresh closure on every
recursive call — i.e. once per node in the body
.

Hoisting that callback to a single visitChild closure (allocated once and
reused for the whole walk) removes the per-node allocation. Behavior is
unchanged — the callback body is identical, it's just no longer re-created at
every node.

var visit func(node *ast.Node)
// allocated once, reused for every node
visitChild := func(child *ast.Node) bool {
    visit(child)
    return false
}
visit = func(node *ast.Node) {
    ...
    node.ForEachChild(visitChild) // was: node.ForEachChild(func(child *ast.Node) bool { visit(child); return false })
}

Benchmark

Synthetic workload: 3,000 void-context callbacks (arr.forEach((x) => { … })),
each with ~20 statements of nested expressions plus a non-void return (so the
rule does not early-out and actually walks the body). Allocations measured with
Go's heap profiler at -memprofilerate=1 (every allocation sampled, so the
counts are exact). The generated source is byte-identical before and after, so
the entire delta is attributable to this change.

Metric Before (inline closure) After (hoisted) Change
alloc_objects 4,439,970 512,916 −3.93M (−88%)
alloc_space 543.6 MB 483.2 MB −60 MB (−11%)
diagnostics 3,000 3,000 unchanged

The per-node closure was being allocated for every node in every body walk
(~3.9M closures across the workload), all of which are eliminated. Each closure
is tiny, so the byte reduction is smaller (~11%), but GC cost is driven largely
by object count — cutting 88% of allocations is a direct reduction in GC
pressure on large/closure-heavy files.

Notes

  • Behavior is unchanged; the rule's existing tests pass and produce an identical
    number of diagnostics.
  • Same pattern (inline ForEachChild/ForEachChildAndJSDoc callback inside a
    recursive walk) exists in a couple of other rules; this PR covers
    strict-void-return only, which walks whole function bodies and so benefits
    the most.

🤖 Generated with Claude Code

@jakebailey

Copy link
Copy Markdown

FWIW I also have microsoft/typescript-go#4395 which will make ForEachChild not escape its func.

@connorshea

connorshea commented Jun 28, 2026

Copy link
Copy Markdown
Member Author

FWIW I also have microsoft/typescript-go#4395 which will make ForEachChild not escape its func.

Thanks for calling that out! Makes sense to me, if we think that's likely to land upstream then I'm fine closing this PR and the other I just opened, since they'll be unnecessary once that change is in typescript-go.

@jakebailey

Copy link
Copy Markdown

I think your PRs are fine; we have a lot of code in TS that does this same thing.

The recursive `visit` that walks a function body declared its ForEachChild
callback inline, heap-allocating a fresh closure on every recursive call
(once per node in the body). Hoist it to a single reused `visitChild`
closure to remove that per-node allocation, reducing GC pressure on large
files. Same pattern as the no-unnecessary-type-parameters fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@connorshea connorshea force-pushed the perf/strict-void-return branch from 5f93a77 to 47d635e Compare June 29, 2026 13:32
@connorshea connorshea marked this pull request as ready for review June 29, 2026 13:32
@connorshea connorshea requested a review from camc314 June 29, 2026 13:32

@camc314 camc314 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.

🚀

Comment thread internal/rules/strict_void_return/strict_void_return.go Outdated
Signed-off-by: Cameron <cameron.clark@hey.com>

@camc314 camc314 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.

🚀

@camc314 camc314 enabled auto-merge (squash) July 1, 2026 18:58
@camc314 camc314 merged commit f87b9aa into main Jul 1, 2026
8 checks passed
@camc314 camc314 deleted the perf/strict-void-return branch July 1, 2026 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants