Don't allocate a VisitorState when a memoized lookup hits the cache - #6078
Closed
vlsi wants to merge 1 commit into
Closed
Don't allocate a VisitorState when a memoized lookup hits the cache#6078vlsi wants to merge 1 commit into
vlsi wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
VisitorState.Cache.get replaced the caller's state with a pathless copy before it looked in the cache, so every read of a memoized value allocated a VisitorState, including the reads the cache answered on its own. Only impl.get needs that copy, so compute now makes it. Compiling the 948 sources of error_prone_core with Error Prone enabled, the compiling thread allocates 7.74 GiB instead of 8.25 GiB. Cache.get runs 25448504 times over that compilation and answers 17019874 of those from the cache, which at 32 bytes per VisitorState accounts for the whole difference. Assisted-by: Claude Code (claude-opus-5)
vlsi
force-pushed
the
vs/memoize-no-alloc-on-hit
branch
from
August 31, 2026 07:51
d2eb1e4 to
e91d4c0
Compare
cpovirk
approved these changes
Aug 31, 2026
cpovirk
left a comment
Member
There was a problem hiding this comment.
FWIW, an internal benchmark on com.google.common.collect doesn't show any improvement in wall time, CPU time, or peak memory. But things can be different when compiling different code and when compiling with different settings, and steady-state memory matters, too. This seems straightforward enough to be worth trying.
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.
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.