Eliminate caching of many collection views in many ImmutableMap implementations.#8570
Open
copybara-service[bot] wants to merge 1 commit into
Open
Eliminate caching of many collection views in many ImmutableMap implementations.#8570copybara-service[bot] wants to merge 1 commit into
ImmutableMap implementations.#8570copybara-service[bot] wants to merge 1 commit into
Conversation
…lementations. This CL removes all fields that held cached views from the base `ImmutableMap` type and from `SingletonImmutableBiMap`. This covers common cases like `ImmutableMap.of(...)`. This CL does _push fields down_ from `ImmutableMap` into a lesser-used branch of the implementation tree, `IteratorBasedImmutableMap`. `IteratorBasedImmutableMap` continues to be used by APIs like `immutableEnumMap`, and those continue to cache their views. It would probably make sense to eliminate caching for many of those implementations, as well, but I wanted to focus on the more commonly used types first, taking care _not_ to remove caching from views that are more heavyweight to compute. (But see a point below about `asMultimap().inverse()`!) (Also: This CL also does not touch GWT/J2CL.) The main motivation for eliminating view caching is to save memory, but: - It likely also helps with J2ObjC memory management. Notably, `ImmutableMap.multimapView` has been set up to use the `@WeakOuter` class `MapViewOfValuesAsSingletonSets`, while the safer approach would likely have been `@RetainedWith`, as seen in cl/781580713. By breaking the reference cycle, this CL eliminates the need for either approach. - Cached views have been a complication for our internal memory tests, as we saw most recently in 0464680. If something were to go wrong with this CL, here are a couple possibilities for what it might be: - Performance might get worse in some unusually allocation-sensitive use case, perhaps involving Android. We could address any Android trouble by rolling back this CL's change to the Android flavor. But it's possible that this CL's impact on Android will be positive, and I'd rather not introduce diffs between the flavors if we don't need to. (That said, the immutable collections are one case in which significant diffs exist already.) - `someImmutableMap.asMultimap().inverse()` can be expensive (specifically, the `inverse()` part of the call chain), so anyone who is using it repeatedly on the same object might be benefiting from the caching of the multimap view. (Kudos to cl/35373012 for having documented this!) I'm hoping that this won't be a big deal, but I plan to monitor fleet-wide profiling for the cost of calls to `ImmutableMultimap.inverse()` after submission. (This CL also happened to naturally eliminate at least one https://errorprone.info/bugpattern/AssignmentExpression warning in `ImmutableMap`, and I ended up eliminating the others in the class (though not the one in `RegularImmutableBiMap`) in response to review feedback. Plus, I suppressed an old https://errorprone.info/bugpattern/PreferredInterfaceType warning on `ImmutableMap.values()`, which we've discussed before in #3255 and b/160392629.) RELNOTES=`collect`: Changed some `ImmutableMap` implementations to create a new, lightweight object on each call to view methods like `keySet()`, rather than creating and storing that object during the first call for reuse later. This is likely to slightly improve performance for some users, but it could lead to regressions in unusual cases, such as repeatedly calling `asMultimap().inverse()` on the same `ImmutableMap` instance. PiperOrigin-RevId: 889873944
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.
Eliminate caching of many collection views in many
ImmutableMapimplementations.This CL removes all fields that held cached views from the base
ImmutableMaptype and fromSingletonImmutableBiMap. This covers common cases likeImmutableMap.of(...).This CL does push fields down from
ImmutableMapinto a lesser-used branch of the implementation tree,IteratorBasedImmutableMap.IteratorBasedImmutableMapcontinues to be used by APIs likeimmutableEnumMap, and those continue to cache their views. It would probably make sense to eliminate caching for many of those implementations, as well, but I wanted to focus on the more commonly used types first, taking care not to remove caching from views that are more heavyweight to compute. (But see a point below aboutasMultimap().inverse()!)(Also: This CL also does not touch GWT/J2CL.)
The main motivation for eliminating view caching is to save memory, but:
ImmutableMap.multimapViewhas been set up to use the@WeakOuterclassMapViewOfValuesAsSingletonSets, while the safer approach would likely have been@RetainedWith, as seen in cl/781580713. By breaking the reference cycle, this CL eliminates the need for either approach.If something were to go wrong with this CL, here are a couple possibilities for what it might be:
someImmutableMap.asMultimap().inverse()can be expensive (specifically, theinverse()part of the call chain), so anyone who is using it repeatedly on the same object might be benefiting from the caching of the multimap view. (Kudos to cl/35373012 for having documented this!) I'm hoping that this won't be a big deal, but I plan to monitor fleet-wide profiling for the cost of calls toImmutableMultimap.inverse()after submission.(This CL also happened to naturally eliminate at least one https://errorprone.info/bugpattern/AssignmentExpression warning in
ImmutableMap, and I ended up eliminating the others in the class (though not the one inRegularImmutableBiMap) in response to review feedback. Plus, I suppressed an old https://errorprone.info/bugpattern/PreferredInterfaceType warning onImmutableMap.values(), which we've discussed before in #3255 and b/160392629.)RELNOTES=
collect: Changed someImmutableMapimplementations to create a new, lightweight object on each call to view methods likekeySet(), rather than creating and storing that object during the first call for reuse later. This is likely to slightly improve performance for some users, but it could lead to regressions in unusual cases, such as repeatedly callingasMultimap().inverse()on the sameImmutableMapinstance.