Skip to content

perf: skip comment assignment when no comments exist#1473

Open
jbedard wants to merge 2 commits into
bazelbuild:mainfrom
jbedard:skip-comment-assignment-when-no-comments
Open

perf: skip comment assignment when no comments exist#1473
jbedard wants to merge 2 commits into
bazelbuild:mainfrom
jbedard:skip-comment-assignment-when-no-comments

Conversation

@jbedard

@jbedard jbedard commented Jun 10, 2026

Copy link
Copy Markdown

In a large repo this made a minor difference in memory consumption on gazelle invocations. This is minor enough it may not be worth it, or maybe only the first commit is worth the extra code.

The first commit is simpler and has the largest gain in my large repo, but doesn't cover as wide of a range of cases.

Robot summary of results:

  ┌────────────────────────────────────────────────────┬───────────────┬─────────┐
  │                       build                        │ order() alloc │ vs base │
  ├────────────────────────────────────────────────────┼───────────────┼─────────┤
  │ base (no buildtools change)                        │ 137.6 MB      │ —       │
  ├────────────────────────────────────────────────────┼───────────────┼─────────┤
  │ commit 1 (guard: skip when no comments)            │ 60.7 MB       │ −56%    │
  ├────────────────────────────────────────────────────┼───────────────┼─────────┤
  │ commit 1 + 2 (guard + finer-grained split)         │ 42.1 MB       │ −69%    │
  └────────────────────────────────────────────────────┴───────────────┴─────────┘

Buildtools PR checklist

  • The code in this PR is covered by unit/integration tests.
  • I have tested these changes and provide testing instructions below.
  • I have either responded to, or resolved all Gemini comments on the PR.
  • I have read Google Eng Practices on Small Changes, this PR either follows these guidelines or the description provides reasoning for why they can not be followed.

Description

assignComments previously walked the full syntax tree on every parse, building preorder and postorder node lists used to attach line and suffix comments.

This change skips the walking and pre/post construction when no comments exists.

(optional) These changes were tested using the following steps

bazel tests, patching the go.mod when invoking gazelle in a large repo while profiling memory/GC

jbedard added 2 commits June 9, 2026 16:48
assignComments calls order() which walks the entire tree appending every
expression to the preorder/postorder lists used only to attach comments. When
the file has no comments this work (and its allocation) is wasted, so return
early.
order() builds both the preorder and postorder lists, but the preorder list is
only consumed when attaching line comments and the postorder list only when
attaching suffix comments. Build each list only when a comment of that kind is
present, so a file with only one kind of comment no longer allocates the unused
list.
@jbedard jbedard requested a review from a team as a code owner June 10, 2026 00:38
@jbedard jbedard requested review from vladmos and removed request for a team June 10, 2026 00:38

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request optimizes the comment assignment process in build/lex.go by skipping the AST traversal in assignComments if there are no line or suffix comments. It also conditionally populates the preorder and postorder lists in order only when the respective comment types are present. No review comments were provided, and I have no feedback to offer.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

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.

1 participant