Cache unresolved ReferenceType lookups - #1837
Merged
Merged
Conversation
A ReferenceType that can't be resolved (eg. a symbol from another file, while processSymbolInformation walks a file on its own) was looked up again on every property access through the proxy - and checks like isCallableType read `kind` many times. A miss can't turn into a hit until a symbol table changes or a scope gets linked, which is what already invalidates the cached hit, so cache misses the same way. Still don't cache when the provider has no table yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
markwpearce
added
the
create-package
Contributor
|
Hey there! I just built a new temporary npm package based on c046762. You can download it here or install it by running the following command: npm install https://github.com/rokucommunity/brighterscript/releases/download/v0.0.0-packages/brighterscript-1.0.0-alpha.55-symbol-information-perf.20260925014927.tgz |
Contributor
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
Updates ReferenceType resolution caching to avoid repeated symbol table lookups (including caching misses) and adds unit tests to validate the new caching behavior.
Changes:
- Cache both successful resolutions and missing-symbol lookups (with invalidation via cache token + global mutation count).
- Refactor
resolveFromTableto take a symbol table instance (and avoid caching when no table is available). - Expand spec coverage for “miss caching”, mutation invalidation, and “no table yet” behavior.
| File | Description |
|---|---|
| src/types/ReferenceType.ts | Refines resolution caching and resolveFromTable signature to reduce repeated lookups and cache misses safely. |
| src/types/ReferenceType.spec.ts | Adds tests asserting resolution after symbol insertion, miss caching/invalidation, and no-table-yet behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Fixes #1835
A ReferenceType that doesn't resolve (eg. a symbol from another file, while
processSymbolInformationwalks a file before it's linked to a scope) got looked up again on every property access through the proxy -processTypeChain'sisCallableTypechecks alone readkind~10 times per chain entry. A miss can't become a hit until a symbol table changes or a scope gets linked, which already invalidates the cached hit from #1831, so misses are cached the same way now (except when the provider has no table yet).Cold validate is 10-28% faster across the benchmark projects, edits up to ~35% faster, diagnostics identical.
processSymbolInformationitself is about half what it was.🤖 Generated with Claude Code