Avoid per-request HashSet allocation in RelativeLoadBalancerStrategy - #1170
Conversation
| _stateUpdater.updateState( | ||
| _enableRelativeStrategyDeferredAllocation ? trackerClients.values() : new HashSet<>(trackerClients.values()), | ||
| partitionId, clusterGenerationId, shouldForceUpdate); |
There was a problem hiding this comment.
After @TylerHorth 's recent changes (#1151), it seems that the map of clients is also mutated from an async thread. In general too, may not be a good idea to assume that the map of tracker clients supplied here is immutable.
There was a problem hiding this comment.
Actually it appears that this map will be immutable. Is it possible to use ImmutableMap for making this clear?
There was a problem hiding this comment.
In the potential client cache, yes it's immutable as well, and it does a value swap when the lb state is updated, such that it's safe for us to do a .values here. Unfortunately we don't have access to any lib to indicate immutability in the interface (guava is not pulled into the classpath for example)
3 participants
JFR identified the HashSet copy constructor call in
RelativeLoadBalancerStrategy.getTrackerClientto be a hotspot since this is executed once per request. The copied set is only consumed by the executor task on cluster changes, not by the steady-state path, such that the construction is wasted most of the time.Since the map that backs the TrackerClient values are not mutated and have no duplicates, defer the construction of the set to when it's actually necessary.
Bonus: fix flaky test in StateUpdaterTest