Don't allocate a VisitorState when a memoized lookup hits the cache - #6082
Merged
Conversation
## Why
`VisitorState.Cache.get` replaced the caller's state with a pathless copy before it looked in the cache:
```java
public synchronized T get(VisitorState state) {
state = state.withNoPathForMemoization();
T value = cache.get();
if (value == null) {
value = impl.get(state);
...
```
The copy keeps a memoized supplier from reading a `TreePath` that belongs to one compilation unit while its result is cached for the whole compilation, so `impl.get` is the only caller that needs it. The cache answers most reads without calling `impl`, and those reads allocated a `VisitorState` and dropped it.
The scanner reaches memoized suppliers through `TypePredicates`, `Suppliers.typeFromString`, `Matchers`, and `MethodMatchers`, once per matcher per AST node, so the reads are frequent: compiling the 948 sources of `error_prone_core` with Error Prone enabled runs `Cache.get` 25448504 times and answers 17019874 of those from the cache.
## What
`compute` now makes the pathless copy, and it runs only when the value has to be computed. `withNoPathForMemoization` returns `this` when the path is already null, so a memoized supplier that reads another memoized value allocates nothing either.
`impl` still receives a state whose `getPath()` throws, and the `provenance` bookkeeping reads `sharedState`, which the copy shares with the original.
## How to verify
```bash
mvn -pl check_api,core test
```
Allocation was measured with an in-process `javac` compiling the 948 sources of `error_prone_core` with Error Prone enabled on JDK 24, reading `ThreadMXBean.getThreadAllocatedBytes` for the compiling thread. Steady state over six compilations in one JVM: 8.251 GiB before, 7.744 GiB after. The 17019874 cached reads at 32 bytes per `VisitorState` come to 0.507 GiB, which is the whole difference.
Fixes #6078
COPYBARA_INTEGRATE_REVIEW=#6078 from vlsi:vs/memoize-no-alloc-on-hit e91d4c0
PiperOrigin-RevId: 973878054
copybara-service
Bot
force-pushed
the
test_973864464
branch
from
August 31, 2026 14:49
c962c73 to
9fefe41
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.
Don't allocate a VisitorState when a memoized lookup hits the cache
Why
VisitorState.Cache.getreplaced the caller's state with a pathless copy before it looked in the cache:The copy keeps a memoized supplier from reading a
TreePaththat belongs to one compilation unit while its result is cached for the whole compilation, soimpl.getis the only caller that needs it. The cache answers most reads without callingimpl, and those reads allocated aVisitorStateand dropped it.The scanner reaches memoized suppliers through
TypePredicates,Suppliers.typeFromString,Matchers, andMethodMatchers, once per matcher per AST node, so the reads are frequent: compiling the 948 sources oferror_prone_corewith Error Prone enabled runsCache.get25448504 times and answers 17019874 of those from the cache.What
computenow makes the pathless copy, and it runs only when the value has to be computed.withNoPathForMemoizationreturnsthiswhen the path is already null, so a memoized supplier that reads another memoized value allocates nothing either.implstill receives a state whosegetPath()throws, and theprovenancebookkeeping readssharedState, which the copy shares with the original.How to verify
mvn -pl check_api,core testAllocation was measured with an in-process
javaccompiling the 948 sources oferror_prone_corewith Error Prone enabled on JDK 24, readingThreadMXBean.getThreadAllocatedBytesfor the compiling thread. Steady state over six compilations in one JVM: 8.251 GiB before, 7.744 GiB after. The 17019874 cached reads at 32 bytes perVisitorStatecome to 0.507 GiB, which is the whole difference.Fixes #6078
FUTURE_COPYBARA_INTEGRATE_REVIEW=#6078 from vlsi:vs/memoize-no-alloc-on-hit e91d4c0