Memoize getName/getNameParts on namespaced AST nodes#1685
Closed
chrisdp wants to merge 1 commit into
Closed
Conversation
Validators call `NamespacedVariableNameExpression.getName`, `NamespaceStatement.getName`, and the corresponding methods on `FunctionStatement`, `ClassStatement`, and `InterfaceStatement` per potential namespace reference per scope. Each call walks the AST (`findAncestor` for the parent namespace) and recursively rebuilds the fully-qualified name string. Results are pure functions of the parsed AST and never change, so caching them on the node is safe. Profiled on a large brighterscript build (~1300 source files): - Total CPU sampled time: 13.58 s -> 12.03 s (-1.55 s, -11.4%) - Validate phase wall-clock: 8.74 s -> 7.60 s (-1.14 s, -13%) - GC self-time: 1.37 s -> 1.08 s (-290 ms, -21%) - Heap allocations: 876 MB -> 869 MB (essentially flat) `getNameParts` returns a defensive slice of the cached array because two existing callers (`BrsFile.ts:999` and `symbolUtils.ts:141`) mutate the result via `.shift()` and `.pop()`. The slice cost is trivial relative to the AST walk it replaces. For Statement-level `getName`, the cache is only written when `this.parent` is set. Without that guard, `NamespaceStatement`'s constructor (which calls `this.name` to build the SymbolTable label before AST walk runs) would lock in the parentless local name forever. `AstNode.spec` clone-equality test updated to skip `_cached*` slots: the cache populates lazily and need not match between a node and its clone. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Member
|
I don't really want to do this at this time. Too much difficulty knowing when to clear these cached values anytime AST edits are made. We need to think about this more and come up with a better more broad caching system. |
3 tasks
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.
Summary
Memoize the result of
getName/getNamePartsonNamespacedVariableNameExpression,NamespaceStatement,FunctionStatement,ClassStatement, andInterfaceStatement. Validators call these methods per potential namespace reference per scope, and each call walks the AST (findAncestor) and rebuilds the qualified name string from scratch. The result is a pure function of the parsed AST, so caching it on the node is safe and eliminates a large amount of repeated work.Measured impact
Profiled on a brighterscript build of ~1300 source files (using
node --cpu-prof --heap-prof):CPU profile attribution before this change:
NamespacedVariableNameExpression.getName455 ms self,getNameParts247 ms self,Statement.getNameand the(get name)getter chain together another ~800 ms self. After this change those frames drop out of the top-25 entirely.Heap stays essentially flat: each relevant AST node grows by two
string | undefinedslots (~16 bytes), but that's offset by the eliminated intermediate string and array allocations from repeated calls.Implementation notes
getNamePartsreturns a defensive slice. Two existing callers mutate the returned array —BrsFile.ts:999uses.shift(),symbolUtils.ts:141uses.pop(). The slice on each call is trivial relative to the AST walk it replaces, and preserves the original mutable-array contract for any external callers.Statement-level
getNameonly writes to the cache whenthis.parentis set. Without that guard,NamespaceStatement's constructor (which callsthis.nameto build a SymbolTable label before the AST walk that establishes parent links) would lock in the parentless local name forever. The guard defers caching until after parent linking, so a nested namespace correctly caches its full qualified name on first post-walk read.Cache fields use the
_cached*prefix.AstNode.spec's clone-equality test was updated to skip those slots: the cache populates lazily and need not match between a node and its clone (each populates independently on first read).🤖 Generated with Claude Code