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) |
| @@ -280,7 +280,7 @@ public static <K, V> Map<K, V> newMapWithExpectedSize(int expectedSize) { | |||
| * @return a new pre-sized {@link HashMap} | |||
| */ | |||
| 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?
| @@ -292,7 +292,7 @@ public static <K, V> Map<K, V> newHashMapWithExpectedSize(int expectedSize) { | |||
| * @return a new pre-sized {@link HashMap} | |||
| */ | |||
| 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);
| @@ -293,25 +293,25 @@ public void testToXContentWithMultipleProjects() throws IOException { | |||
| }, | |||
| "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 <--
| @@ -280,7 +280,7 @@ public static <K, V> Map<K, V> newMapWithExpectedSize(int expectedSize) { | |||
| * @return a new pre-sized {@link HashMap} | |||
| */ | |||
| 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?
| @@ -292,7 +292,7 @@ public static <K, V> Map<K, V> newHashMapWithExpectedSize(int expectedSize) { | |||
| * @return a new pre-sized {@link HashMap} | |||
| */ | |||
| 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.
This change simplifies pre-sizing the map.