start Interning instances of SlsVersionMatcher #942
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.
Before this PR
I was looking at heap histograms of apollo-catalog and noticed we're storing ~286 megabytes of this one
SlsVersionMatcher
class.Given that I think the vast majority of SlsVersionMatchers come from a small number of constants (e.g.
1.x.x
,2.x.x
,3.x.x
) I figured we could eliminate pretty much all of this memory usage by just interning them.I started looking at the
@value.immutables
feature of weak interning https://immutables.github.io/immutable.html#instance-interning, but the generated code just used a ConcurrentHashMap under the hood, which made me worry about throughput. (Maybe this is because we have thejdkOnly = true
flag on our ImmutablesStyle?)It did however have a neat optimization for the equals method, where it just checks reference equality (i.e.
==
) and uses a separate.isEqualTo
method which does the real equals check just for the interner.After this PR
==COMMIT_MSG==
start Interning instances of SlsVersionMatcher
==COMMIT_MSG==
HOWEVER I've added
do not merge
because I am a bit worried about the implications of these leading zeros in the numbers. This seems to be arising because we've marked user input string as@Value.Auxiliary
(so it doesn't contribute to the equals check) and we're just relying on the other fields to determine equality.I think we have a few options:
@Value.Auxiliary
annotation from theString getValue()
and put it on all the other fields, so that "01.0.0" is genuinely considered not equal to "1.0.0". I think this leads to weird implications because the.compare
method would return 0.Possible downsides?