Skip to content
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

Jump to bottom

Remove uses of ImmutableOpen*Map #86239

Open
14 of 35 tasks
rjernst opened this issue Apr 27, 2022 · 5 comments
Open
14 of 35 tasks

Remove uses of ImmutableOpen*Map #86239

rjernst opened this issue Apr 27, 2022 · 5 comments
Labels
:Core/Infra/Core Core issues without another label Meta Team:Core/Infra Meta label for core/infra team >tech debt

Comments

@rjernst
Copy link
Member

rjernst commented Apr 27, 2022 •
edited by thecoop
Loading

After #84735, hppc usage in Elasticsearch is now isolated to ImmutableOpenMap and ImmutableOpenIntMap. This is a meta issue delineating the remaining uses of those classes that need to be removed.
@rjernst rjernst added :Core/Infra/Core Core issues without another label Meta labels Apr 27, 2022
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Apr 27, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@rjernst rjernst mentioned this issue Apr 27, 2022
Closed
43 tasks
rjernst added a commit to rjernst/elasticsearch that referenced this issue Apr 28, 2022
@rjernst
Remove ImmutableOpen*Map from Version
1f35dcf 
The Version class has two internal static maps for keeping track of the
known Version objects and quickly decoding from a version id into the
known Version object. This commit converts these maps to HashMaps.

relates elastic#86239
@rjernst rjernst mentioned this issue Apr 28, 2022
Merged
@dakrone
Copy link
Member

dakrone commented Apr 28, 2022

@rjernst do we still plan to keep some (most?) of these as immutable maps, or are we converting to mutable HashMap-like maps for these?

@rjernst
Copy link
Member Author

rjernst commented Apr 28, 2022

Yes I think they can and should remain immutable, just immutable Map instances with Map.of or Map.copyOf.

rjernst added a commit that referenced this issue Apr 28, 2022
@rjernst
Remove ImmutableOpen*Map from Version (#86245)
74e9d7c 
The Version class has two internal static maps for keeping track of the
known Version objects and quickly decoding from a version id into the
known Version object. This commit converts these maps to HashMaps.

relates #86239
rjernst added a commit to rjernst/elasticsearch that referenced this issue Apr 29, 2022
@rjernst
Convert IndicesShardStoreResponse to use Map
2268846 
The IndicesShardStoreResponse class uses ImmutableOpenMap and
ImmutableOpenIntMap to represent the per index -> shard -> status
structure. This commit converts these to Java Maps.

relates elastic#86239
@rjernst rjernst mentioned this issue Apr 29, 2022
Merged
rjernst added a commit that referenced this issue Apr 29, 2022
@rjernst
Convert IndicesShardStoreResponse to use Map (#86281)
1c836da 
The IndicesShardStoreResponse class uses ImmutableOpenMap and
ImmutableOpenIntMap to represent the per index -> shard -> status
structure. This commit converts these to Java Maps.

relates #86239
rjernst added a commit to rjernst/elasticsearch that referenced this issue Apr 29, 2022
@rjernst
Remove ImmutableOpenIntMap from ClusterStateHealthTests
c55187b 
These tests construct a map of shard to allocation ids. But they only do
so get the pairs to put into the index metadata builder. This commit
converts it to use HashMap.

relates elastic#86239
@rjernst rjernst mentioned this issue Apr 29, 2022
Merged
rjernst added a commit that referenced this issue May 2, 2022
@rjernst
Remove ImmutableOpenIntMap from ClusterStateHealthTests (#86306)
fbe1c59 
These tests construct a map of shard to allocation ids. But they only do
so get the pairs to put into the index metadata builder. This commit
converts it to use HashMap.

relates #86239
@joegallo joegallo mentioned this issue May 2, 2022
Merged
rjernst added a commit to rjernst/elasticsearch that referenced this issue May 2, 2022
@rjernst
Add readImmutableMap for Java Map
fa4d4f1 
The readImmutableMap method of StreamInput reads an ImmutableOpenMap. As
we work towards elastic#86239, we want to continue using immutable maps, but
instead use those from the JDK. This commit renames the existing
readImmutableMap to readImmutableOpenMap, and reimplements the existing
method to return an immutable Map using Map.of and Map.ofEntries.

relates elastic#86239
@rjernst rjernst mentioned this issue May 2, 2022
Merged
rjernst added a commit to rjernst/elasticsearch that referenced this issue May 3, 2022
@rjernst
Remove use of ImmutableOpenIntMap in IndexRoutingTable
14d85b7 
The routing table builder uses an hppc based immutable map to keep track
of shard id to shard table. However, when building the final routing
table, it is just converted to an array. This commit switches to a
HashMap.

relates elastic#86239
@rjernst rjernst mentioned this issue May 3, 2022
Merged
rjernst added a commit that referenced this issue May 3, 2022
@rjernst
Remove use of ImmutableOpenIntMap in IndexRoutingTable (#86368)
3e02748 
The routing table builder uses an hppc based immutable map to keep track
of shard id to shard table. However, when building the final routing
table, it is just converted to an array. This commit switches to a
HashMap.

relates #86239
rjernst added a commit to rjernst/elasticsearch that referenced this issue May 3, 2022
@rjernst
Use HashMap for IndexMetadata.inSyncAllocationIds
5e1b6de 
The in sync allocation ids is a mapping from shard to the set of ids
currently being processed. When being built, the metadata uses a sparse
map, only filling a value for a shard as they are put into the builder.
When the final metadata is built, the map is made dense.

This commit converts to using a HashMap instead of ImmutableOpenIntMap.
The boxed keys should not cause allocations, as long as number of shards
is lower than 128. Long term the map itself should become an array, as
we know the number of shards (much like the dense primaryTerms array
here). However, that change will be a little trickier to make, since we
will need to be backward compatible with how diffs are built, currently
using Map differences.

relates elastic#86239
@rjernst rjernst mentioned this issue May 3, 2022
Merged
rjernst added a commit that referenced this issue May 4, 2022
@rjernst
Add readImmutableMap for Java Map (#86353)
2daf287 
The readImmutableMap method of StreamInput reads an ImmutableOpenMap. As
we work towards #86239, we want to continue using immutable maps, but
instead use those from the JDK. This commit renames the existing
readImmutableMap to readImmutableOpenMap, and reimplements the existing
method to return an immutable Map using Map.of and Map.ofEntries.

relates #86239
rjernst added a commit to rjernst/elasticsearch that referenced this issue May 4, 2022
@rjernst
Remove remaining ImmutableOpen maps from shard stores response
16c0197 
The IndicesShardStoresResponse had ImmutableOpenInt map removed, but its
use accidentally remained in serialization. Thankfully the format for
immutable open maps are interchangable with read java Map. This commit
switches to using the new readImmutableMap to read the store statuses.

relates elastic#86239
@rjernst rjernst mentioned this issue May 4, 2022
Merged
rjernst added a commit that referenced this issue May 4, 2022
@rjernst
Use HashMap for IndexMetadata.inSyncAllocationIds (#86403)
2e74205 
The in sync allocation ids is a mapping from shard to the set of ids
currently being processed. When being built, the metadata uses a sparse
map, only filling a value for a shard as they are put into the builder.
When the final metadata is built, the map is made dense.

This commit converts to using a HashMap instead of ImmutableOpenIntMap.
The boxed keys should not cause allocations, as long as number of shards
is lower than 128. Long term the map itself should become an array, as
we know the number of shards (much like the dense primaryTerms array
here). However, that change will be a little trickier to make, since we
will need to be backward compatible with how diffs are built, currently
using Map differences.

relates #86239
rjernst added a commit that referenced this issue May 4, 2022
@rjernst
Remove remaining ImmutableOpen maps from shard stores response (#86427)
f049611 
The IndicesShardStoresResponse had ImmutableOpenInt map removed, but its
use accidentally remained in serialization. Thankfully the format for
immutable open maps are interchangable with read java Map. This commit
switches to using the new readImmutableMap to read the store statuses.

relates #86239
@rjernst rjernst mentioned this issue Jun 21, 2022
Closed
rjernst added a commit to rjernst/elasticsearch that referenced this issue Jun 21, 2022
@rjernst
Use HashMap inside cluster info service
10495a2 
The InternalClusterInfoService internally uses ImmutableOpenMap
for keeping track of available space on each node. This commit converts
those usages to use HashMap. Note that an unmodifiableMap wrapper is
used because updates to this (from each node) are likely to happen often
as disk is used.

relates elastic#86239
@rjernst rjernst mentioned this issue Jun 21, 2022
Merged
rjernst added a commit that referenced this issue Jun 22, 2022
@rjernst
Use HashMap inside cluster info service (#87899)
f627bdd 
The InternalClusterInfoService internally uses ImmutableOpenMap
for keeping track of available space on each node. This commit converts
those usages to use HashMap. Note that an unmodifiableMap wrapper is
used because updates to this (from each node) are likely to happen often
as disk is used.

relates #86239
rjernst added a commit to rjernst/elasticsearch that referenced this issue Jun 22, 2022
@rjernst
Use HashMap in admin get actions
99a2e34 
Actions for getting aliases, indices, and settings all return a Map. Yet
internally these unnecessarily ImmutableOpenMap. This commit converts
these actions to use HashMap.

relates elastic#86239
@rjernst rjernst mentioned this issue Jun 22, 2022
Merged
elasticsearchmachine pushed a commit that referenced this issue Jun 24, 2022
@rjernst
Use HashMap in admin get actions (#87938)
981a345 
Actions for getting aliases, indices, and settings all return a Map. Yet
internally these unnecessarily ImmutableOpenMap. This commit converts
these actions to use HashMap.

relates #86239
rjernst added a commit to rjernst/elasticsearch that referenced this issue Jul 5, 2022
@rjernst
Remove ImmutableOpenMap from most tests
e58254f 
This commit removes ImmutableOpenMap from most test cases from server,
where the type is no longer needed because Map is used by the class
being constructed.

relates elastic#86239
@rjernst rjernst mentioned this issue Jul 5, 2022
Merged
rjernst added a commit that referenced this issue Jul 5, 2022
@rjernst
Remove ImmutableOpenMap from most tests (#88285)
a9d1986 
This commit removes ImmutableOpenMap from most test cases from server,
where the type is no longer needed because Map is used by the class
being constructed.

relates #86239
rjernst added a commit to rjernst/elasticsearch that referenced this issue Jul 5, 2022
@rjernst
Remove ImmutableOpenMap from rollupt tests
c3ac2d7 
Some tests for rollup were still using ImmutableOpenMap for testing
internal methods, even though those methods were already converted to
Map. This commit changes those tests to use Map.

relates elastic#86239
@rjernst rjernst mentioned this issue Jul 5, 2022
Merged
rjernst added a commit to rjernst/elasticsearch that referenced this issue Jul 5, 2022
@rjernst
Remove ImmutableOpenMap from Engine
6fec303 
SegmentStats was changed to use Map, but the method in Engine which
computes files sizes for segment stats was never converted. This commit
removes that final usage from Engine.

relates elastic#86239
@rjernst rjernst mentioned this issue Jul 5, 2022
Merged
rjernst added a commit that referenced this issue Jul 5, 2022
@rjernst
Remove ImmutableOpenMap from Engine (#88291)
bfd24f9 
SegmentStats was changed to use Map, but the method in Engine which
computes files sizes for segment stats was never converted. This commit
removes that final usage from Engine.

relates #86239
rjernst added a commit that referenced this issue Jul 5, 2022
@rjernst
Remove ImmutableOpenMap from rollupt tests (#88290)
51fa9e3 
Some tests for rollup were still using ImmutableOpenMap for testing
internal methods, even though those methods were already converted to
Map. This commit changes those tests to use Map.

relates #86239
rjernst added a commit to rjernst/elasticsearch that referenced this issue Jul 9, 2022
@rjernst
Make DiffableUtils.diff implementation agnostic
6983303 
DiffableUtils has methods for reading and creating diffs of maps. The
two implementation types, a JDK Map or ImmutableOpenMap, have different
methods and implementations. However, the format of the diff is always
the same, so the diff creation can be shared. This commit creates an
internal MapBuilder abstraction to allow both Map and ImmutableOpenMap
to produce diffs from the same MapDiff implementation.

relates elastic#86239
@rjernst rjernst mentioned this issue Jul 9, 2022
Merged
rjernst added a commit that referenced this issue Jul 12, 2022
@rjernst
Make DiffableUtils.diff implementation agnostic (#88403)
5d23480 
DiffableUtils has methods for reading and creating diffs of maps. The
two implementation types, a JDK Map or ImmutableOpenMap, have different
methods and implementations. However, the format of the diff is always
the same, so the diff creation can be shared. This commit creates an
internal MapBuilder abstraction to allow both Map and ImmutableOpenMap
to produce diffs from the same MapDiff implementation.

relates #86239
@thecoop thecoop mentioned this issue Sep 7, 2022
Merged
thecoop added a commit that referenced this issue Sep 8, 2022
@thecoop
Remove ImmutableOpenMap from some server tests (#89864)
6807b69 
Remove ImmutableOpenMap from some server tests

relates #86239
@thecoop
Copy link
Member

thecoop commented Sep 12, 2022

ImmutableOpenMap was added to the cluster/index metadata classes in #86673 for performance reasons. So we need to find a different collection type (maybe guava immutablemap?) if we want to remove ImmutableOpenMap

@thecoop thecoop mentioned this issue Sep 12, 2022
Merged
@thecoop
Copy link
Member

thecoop commented Sep 23, 2022

#90006 caused a performance drop, needs to be thought about more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
:Core/Infra/Core Core issues without another label Meta Team:Core/Infra Meta label for core/infra team >tech debt
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@dakrone @rjernst @thecoop @elasticmachine @rdnm