Repository navigation
Drop the Flow parser's comment-moving AST mapper classes - #9267
Merged
Merged
Conversation
Changelog💅 Polish
Semantic Diff1 commit · Full page Drop the Flow parser's comment-moving AST mapper classes7d09248 · cknitt + Claude · Nov 08, 2026, 18:03 UTC Flow moves trailing comments off nodes it has already built with an AST mapper class (Flow_ast_mapper, 336 methods) and two subclasses in Comment_attachment. OCaml creates classes when their module is initialized, so every compiler process built their method tables: about 206,000 of the compiler's ~275,000 startup words, although ReScript only parses %raw code with the Flow parser and never reads trailing comments (only the leading comment of a %raw literal, which the parser collects without the mapper). The removers now return the node unchanged; what they kept is the consumption of comments, which decides where later comments attach. Flow_ast_mapper and both classes go. The Flow parser's startup allocation, measured by linking it into an otherwise empty program, drops from 227,172 to 20,669 words. |
Member
Author
|
@codex review |
cknitt
marked this pull request as ready for review
November 8, 2026 18:14
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🎉 Compiler benchmarksAllocated wordsIn total: 804,921,516 → 693,364,295 (-13.86%). 🎉 Improvement: 255 benchmarks allocate more than 1% less.
250 more changes
InstructionsIn total: 10,228,138,742 → 9,755,245,397 (-4.62%). 🎉 Improvement: 30 benchmarks execute more than 1% fewer instructions.
23 more changes
Output sizeIn total: 29,199,436 → 29,109,020 (-0.31%). 🎉 Improvement: 1 output is more than 1% smaller.
Pull request 7d09248 compared with base 06d6fa0 in this run. |
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
fhammerschmidt
approved these changes
Nov 9, 2026
cknitt
force-pushed
the
remove-flow-ast-mapper
branch
from
November 9, 2026 09:06
defd652 to
02aa9d8
Compare
cknitt
added a commit
that referenced
this pull request
Nov 9, 2026
* Stop counting instructions for the project builds A ticker wakes the build's scheduler every 5 ms of wall-clock time, so under cachegrind the builds' instructions depend on how fast the runner is: in #9267 the leaf-edit build grew by 348% and the no-op build by 54% although their allocations didn't change, failing the job. Their allocated words stay, and the job runs faster. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gGPsF3fHZn9rBxA7Hm Signed-off-by: Christoph Knittel <ck@cca.io> * Retrigger the DCO check The DCO app left its check suite for the previous commit queued and never reported; a new commit makes it check the pull request again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gGPsF3fHZn9rBxA7Hm Signed-off-by: Christoph Knittel <ck@cca.io> --------- Signed-off-by: Christoph Knittel <ck@cca.io> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Flow moves trailing comments off nodes it has already built with an AST mapper class (Flow_ast_mapper, 336 methods) and two subclasses in Comment_attachment. OCaml creates classes when their module is initialized, so every compiler process built their method tables: about 206,000 of the compiler's ~275,000 startup words, although ReScript only parses %raw code with the Flow parser and never reads trailing comments (only the leading comment of a %raw literal, which the parser collects without the mapper). The removers now return the node unchanged; what they kept is the consumption of comments, which decides where later comments attach. Flow_ast_mapper and both classes go. The Flow parser's startup allocation, measured by linking it into an otherwise empty program, drops from 227,172 to 20,669 words. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT Signed-off-by: Christoph Knittel <ck@cca.io>
cknitt
force-pushed
the
remove-flow-ast-mapper
branch
from
November 9, 2026 12:02
02aa9d8 to
7d09248
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #9173. Makes the compiler start with much less work by dropping the Flow parser's comment-moving AST mapper classes.
Why
The Flow parser (
compiler/flow_parser) only parses%rawcode, but OCaml initializes all its modules when the compiler starts. ItsFlow_ast_mapper.mapperclass (336 methods) and the twoComment_attachmentclasses that inherit it are created then, method tables and a closure per method, in every compiler process.Measured by linking the Flow parser into an otherwise empty program:
bsc -vallocates in total.This PR's benchmark (see the comment below) shows the effect on whole compilations:
Primitive_list.resi: 568,603 → 122,397).Stdlib_Math.resi: 44.1M → 24.6M).The saving is a fixed amount per compiler process, so small files gain the most;
parse/Napkinscript.resgains 2–3%.What changes
Flow uses the mapper to move trailing comments off nodes it has already built. ReScript never reads trailing comments. It only reads the leading comment of a
%rawliteral, which the parser collects without the mapper, and whichtests/tests/src/raw_pure_test.rescovers.Comment_attachmentnow return the node unchanged.Flow_ast_mapperand both classes are removed, along with the~kindarguments the parsers passed for them.Without the mapper, most fields of the Flow AST are only written; #9208 handles that for
make dead-code.🤖 Generated with Claude Code
https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT