Allow nil-ing out arenas to break the GC link between nodes and the arena they came from#4024
Open
weswigham wants to merge 2 commits into
Open
Allow nil-ing out arenas to break the GC link between nodes and the arena they came from#4024weswigham wants to merge 2 commits into
weswigham wants to merge 2 commits into
Conversation
…rena they came from
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce retained memory during diagnostic/type rendering by explicitly releasing AST node allocation arenas after short-lived node-building operations, breaking GC retention between the cached NodeBuilder/EmitContext and previously allocated nodes.
Changes:
- Add
Arena.Release()to nil out arena backing slices and allow GC to reclaim unused allocations. - Generate
(*ast.NodeFactory).ReleaseArenas()and invoke it via a newreleasecallback returned fromChecker.getNodeBuilder(). - Update various
Checkerstringification/printing helpers todefer release()after using the shared cachedNodeBuilder.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/core/arena.go | Adds Arena.Release() to drop references to arena backing storage. |
| internal/checker/printer.go | Uses getNodeBuilder() release callback in type/symbol/signature stringification paths. |
| internal/checker/nodebuilder.go | Changes getNodeBuilder() to return (builder, release) and releases node factory arenas after use. |
| internal/ast/ast_generated.go | Adds generated NodeFactory.ReleaseArenas() that releases all node arenas. |
| _scripts/generate-go-ast.ts | Updates generator to emit ReleaseArenas() for NodeFactory. |
Files not reviewed (1)
- internal/ast/ast_generated.go: Language not supported
| return slice | ||
| } | ||
|
|
||
| // Reset arena back to zero, removing GC refs between the arena and any objects in the arena, allowing them to be freed |
Comment on lines
202
to
204
| if vc != nil { | ||
| nodeBuilder.verbosity = vc | ||
| } |
Member
Author
There was a problem hiding this comment.
@gabritto @RyanCavanaugh I'm pretty sure this copilot comment just picked up on the fix for #4010 (unrelated to this PR though)
Comment on lines
326
to
328
| if vc != nil { | ||
| nodeBuilder.verbosity = vc | ||
| } |
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.
Which should lower the average retained memory total of diagnostic generation.