[rest.li] Update SimpleLoadBalancer to use for loop instead of Map #947
Conversation
| .collect(Collectors.toMap( | ||
| uri -> uri, | ||
| uri -> uris.getPartitionDataMap(uri).get(partitionId).getWeight())); | ||
| Map<URI, Double> weightedUris = new HashMap<>(possibleUris.size()); |
There was a problem hiding this comment.
do we have a util to calculate the map size according to the load factor? this is to prevent map resizes
There was a problem hiding this comment.
JDK assume standard 66%, so if we want to optimize it to the max and avoid hash collisions and collision chains we can divide possibleUris size by 0.66 and round up to the nearest integer
There was a problem hiding this comment.
When you preallocate the map size like this you dramatically reduce the odds of a resize during the insertion. It's still possible you'd get a resize if for whatever reason there were many hash collisions, but you can't really do anything about that anyway, even if you took into account the default load factor. I guess the only way to truly prevent a resize is to use an ImmutableMap.builder from guava which lazily creates the backing map
There was a problem hiding this comment.
I doubt possibleURIs has hundreds of thousands or millions of elements. I expect <1K, thus chance of URI hash collisions is very low, and thus map size equal to collection size should work fine ;)
[rest.li] Update SimpleLoadBalancer to use for loop instead of Map
When running ODP analysis, we observed that SimpleLoadBalancer.getPotentialClients map collect uses around 4% CPU. Much of this seems Collector.toMap's uniqKeysMapAccumulator. In this scenario, this is performing unnecessary extra computation -- the keys of the Map we are creating come directly from a Set.
The updated implementation avoids the stream(...).collect(Collectors.toMap(...) and directly uses a for loop to populate the Map