Simplify map initial sizing #126767
Simplify map initial sizing #126767
Conversation
idegtiarenko
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| */ | ||
| public static <K, V> Map<K, V> newHashMapWithExpectedSize(int expectedSize) { | ||
| return new HashMap<>(capacity(expectedSize)); | ||
| return HashMap.newHashMap(expectedSize); |
There was a problem hiding this comment.
Since 19 both HashMap and LinkedHashMap could be presized using standard lib factories.
There was a problem hiding this comment.
nit: perhaps then (doesn't have to be here) we should convert all the uses of this helper to use HashMap.newHashMap directly?
| */ | ||
| public static <K, V> Map<K, V> newConcurrentHashMapWithExpectedSize(int expectedSize) { | ||
| return new ConcurrentHashMap<>(capacity(expectedSize)); | ||
| return new ConcurrentHashMap<>(expectedSize); |
There was a problem hiding this comment.
ConcurrentHashMap does this sizing computation internally:
according to the javadoc: Params: initialCapacity – The implementation performs internal sizing to accommodate this many elements.
and constructor code:
long size = (long)(1.0 + (long)initialCapacity / loadFactor);
int cap = (size >= (long)MAXIMUM_CAPACITY) ?
MAXIMUM_CAPACITY : tableSizeFor((int)size);
| "projects": [ | ||
| { | ||
| "id": "tb5W0bx765nDVIwqJPw92G", | ||
| "id": "3LftaL7hgfXAsF60Gm6jcD", |
There was a problem hiding this comment.
It looks like our calculation creates a bigger maps in some cases. This results in a different iteration order in several tests.
There was a problem hiding this comment.
expected size=0 old_capacity=1 new_capacity=0 <--
expected size=1 old_capacity=2 new_capacity=2
expected size=2 old_capacity=3 new_capacity=3
expected size=3 old_capacity=5 new_capacity=4 <--
expected size=4 old_capacity=6 new_capacity=6
expected size=5 old_capacity=7 new_capacity=7
expected size=6 old_capacity=9 new_capacity=8 <--
expected size=7 old_capacity=10 new_capacity=10
expected size=8 old_capacity=11 new_capacity=11
expected size=9 old_capacity=13 new_capacity=12 <--
expected size=10 old_capacity=14 new_capacity=14
expected size=11 old_capacity=15 new_capacity=15
expected size=12 old_capacity=17 new_capacity=16 <--
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
All 3 of these cases seem to suggest we don't need the utility methods anymore?
| */ | ||
| public static <K, V> Map<K, V> newHashMapWithExpectedSize(int expectedSize) { | ||
| return new HashMap<>(capacity(expectedSize)); | ||
| return HashMap.newHashMap(expectedSize); |
There was a problem hiding this comment.
nit: perhaps then (doesn't have to be here) we should convert all the uses of this helper to use HashMap.newHashMap directly?
| */ | ||
| public static <K, V> Map<K, V> newConcurrentHashMapWithExpectedSize(int expectedSize) { | ||
| return new ConcurrentHashMap<>(capacity(expectedSize)); | ||
| return new ConcurrentHashMap<>(expectedSize); |
There was a problem hiding this comment.
Similar to the above note, this utility method doesn't seem to be doing anything anymore, so perhaps it should be removed.
There was a problem hiding this comment.
(here and above comment) Yeap, newMapWithExpectedSize, newHashMapWithExpectedSize, newConcurrentHashMapWithExpectedSize, newLinkedHashMapWithExpectedSize can all be inlined.
5 participants
This change simplifies pre-sizing the map.