Skip to content

Simplify map initial sizing #126767

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Simplify map initial sizing #126767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7729714
Simplify map initial sizing
idegtiarenko Apr 14, 2025
f1c5b6e
fix test projects ordering
idegtiarenko Apr 14, 2025
7dd4636
Merge branch 'main' into map_sizing
idegtiarenko Apr 14, 2025
1bcd3dd
Merge branch 'main' into map_sizing
idegtiarenko Apr 15, 2025
5736036
Merge branch 'main' into map_sizing
idegtiarenko Apr 22, 2025
01114fb
Merge branch 'main' into map_sizing
idegtiarenko Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions server/src/main/java/org/elasticsearch/common/util/Maps.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since 19 both HashMap and LinkedHashMap could be presized using standard lib factories.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: perhaps then (doesn't have to be here) we should convert all the uses of this helper to use HashMap.newHashMap directly?

}

/**
Expand All @@ -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);
Copy link
Contributor Author

@idegtiarenko idegtiarenko Apr 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the above note, this utility method doesn't seem to be doing anything anymore, so perhaps it should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(here and above comment) Yeap, newMapWithExpectedSize, newHashMapWithExpectedSize, newConcurrentHashMapWithExpectedSize, newLinkedHashMapWithExpectedSize can all be inlined.

}

/**
Expand All @@ -304,12 +304,7 @@ public static <K, V> Map<K, V> newConcurrentHashMapWithExpectedSize(int expected
* @return a new pre-sized {@link LinkedHashMap}
*/
public static <K, V> LinkedHashMap<K, V> newLinkedHashMapWithExpectedSize(int expectedSize) {
return new LinkedHashMap<>(capacity(expectedSize));
}

static int capacity(int expectedSize) {
assert expectedSize >= 0;
return expectedSize < 2 ? expectedSize + 1 : (int) (expectedSize / 0.75 + 1.0);
return LinkedHashMap.newLinkedHashMap(expectedSize);
}

/**
Expand Down
96 changes: 48 additions & 48 deletions server/src/test/java/org/elasticsearch/cluster/ClusterStateTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,25 +293,25 @@ public void testToXContentWithMultipleProjects() throws IOException {
},
"projects": [
{
"id": "tb5W0bx765nDVIwqJPw92G",
"id": "3LftaL7hgfXAsF60Gm6jcD",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like our calculation creates a bigger maps in some cases. This results in a different iteration order in several tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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 <--

"indices": {
"common-index": {
"9": {
"another-index": {
"5": {
"retryable": false,
"description": "index metadata (api)",
"levels": [ "metadata_read", "metadata_write"]
"description": "index read-only (api)",
"levels": [ "write", "metadata_write"]
}
}
}
},
{
"id": "3LftaL7hgfXAsF60Gm6jcD",
"id": "tb5W0bx765nDVIwqJPw92G",
"indices": {
"another-index": {
"5": {
"common-index": {
"9": {
"retryable": false,
"description": "index read-only (api)",
"levels": [ "write", "metadata_write"]
"description": "index metadata (api)",
"levels": [ "metadata_read", "metadata_write"]
}
}
}
Expand Down Expand Up @@ -468,40 +468,7 @@ public void testToXContentWithMultipleProjects() throws IOException {
"voting_config_exclusions": []
},
"projects": [
{
"id": "tb5W0bx765nDVIwqJPw92G",
"templates": {},
"indices": {
"common-index": {
"version": 2,
"mapping_version": 1,
"settings_version": 1,
"aliases_version": 1,
"routing_num_shards": 3,
"state": "open",
"settings": {
"index": {
"number_of_shards": "3",
"number_of_replicas": "1",
"uuid": "tE62Ga40yvlmOSujUvruVw",
"version": { "created": "%s" }
}
},
"mappings": {},
"aliases": [],
"primary_terms": { "0":0, "1":0, "2":0 },
"in_sync_allocations": { "0":[], "1":[], "2":[] },
"rollover_info": {},
"mappings_updated_version": %s,
"system": false,
"timestamp_range": { "shards":[] },
"event_ingested_range": { "shards": [] }
}
},
"index-graveyard": { "tombstones": [] },
"reserved_state": {}
},
{
{
"id": "3LftaL7hgfXAsF60Gm6jcD",
"templates": {},
"indices": {
Expand Down Expand Up @@ -559,12 +526,45 @@ public void testToXContentWithMultipleProjects() throws IOException {
"index-graveyard": { "tombstones": [] },
"reserved_state": {}
},
{
{
"id": "WHyuJ0uqBYOPgHX9kYUXlZ",
"templates": {},
"indices": {},
"index-graveyard": { "tombstones": [] },
"reserved_state": {}
},
{
"id": "tb5W0bx765nDVIwqJPw92G",
"templates": {},
"indices": {
"common-index": {
"version": 2,
"mapping_version": 1,
"settings_version": 1,
"aliases_version": 1,
"routing_num_shards": 3,
"state": "open",
"settings": {
"index": {
"number_of_shards": "3",
"number_of_replicas": "1",
"uuid": "tE62Ga40yvlmOSujUvruVw",
"version": { "created": "%s" }
}
},
"mappings": {},
"aliases": [],
"primary_terms": { "0":0, "1":0, "2":0 },
"in_sync_allocations": { "0":[], "1":[], "2":[] },
"rollover_info": {},
"mappings_updated_version": %s,
"system": false,
"timestamp_range": { "shards":[] },
"event_ingested_range": { "shards": [] }
}
},
"index-graveyard": { "tombstones": [] },
"reserved_state": {}
}
],
"reserved_state": {}
Expand Down Expand Up @@ -812,14 +812,14 @@ public void testToXContentWithMultipleProjects() throws IOException {
Version.CURRENT,
IndexVersions.MINIMUM_COMPATIBLE,
IndexVersion.current(),
// project:tb5W0bx765nDVIwqJPw92G index:common-index
IndexVersion.current(),
IndexVersion.current(),
// project:3LftaL7hgfXAsF60Gm6jcD index:another-index
IndexVersion.current(),
IndexVersion.current(),
// project:3LftaL7hgfXAsF60Gm6jcD index:common-index
IndexVersion.current(),
IndexVersion.current(),
// project:tb5W0bx765nDVIwqJPw92G index:common-index
IndexVersion.current(),
IndexVersion.current()
);

Expand Down
17 changes: 0 additions & 17 deletions server/src/test/java/org/elasticsearch/common/util/MapsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.lessThanOrEqualTo;

public class MapsTests extends ESTestCase {

Expand Down Expand Up @@ -270,22 +269,6 @@ public void testFlatten() {
}
}

public void testCapacityIsEnoughForMapToNotBeResized() {
for (int i = 0; i < 1000; i++) {
int size = randomIntBetween(0, 1_000_000);
int capacity = Maps.capacity(size);
assertThat(size, lessThanOrEqualTo((int) (capacity * 0.75f)));
}
}

public void testCapacityForMaxSize() {
assertEquals(Integer.MAX_VALUE, Maps.capacity(Integer.MAX_VALUE));
}

public void testCapacityForZeroSize() {
assertEquals(1, Maps.capacity(0));
}

@SuppressWarnings("unchecked")
private static Object deepGet(String path, Object obj) {
Object cur = obj;
Expand Down