Skip to content

Make shardIndexMap in AbstractSearchAsyncAction a local variable #117168

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

Merged
merged 4 commits into from
Jan 21, 2025
Merged

Make shardIndexMap in AbstractSearchAsyncAction a local variable #117168

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b6c94e1
Make shardIndexMap in AbstractSearchAsyncAction a local variable
original-brownbear Nov 20, 2024
4cc1f9a
Merge branch 'main' into save-needless-big-shardIndexMap
original-brownbear Nov 22, 2024
e3a4dc8
Merge branch 'main' into save-needless-big-shardIndexMap
original-brownbear Dec 10, 2024
ebc12a1
Merge branch 'main' into save-needless-big-shardIndexMap
original-brownbear Jan 15, 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
22 changes: 9 additions & 13 deletions server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
package org.elasticsearch.action.search;

import org.apache.logging.log4j.Logger;
import org.apache.lucene.util.CollectionUtil;
import org.apache.lucene.util.SetOnce;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ExceptionsHelper;
Expand All @@ -26,6 +25,7 @@
import org.elasticsearch.cluster.routing.GroupShardsIterator;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.util.Maps;
import org.elasticsearch.common.util.concurrent.AtomicArray;
import org.elasticsearch.core.Releasable;
import org.elasticsearch.core.Releasables;
Expand All @@ -43,8 +43,7 @@
import org.elasticsearch.transport.Transport;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
Expand Down Expand Up @@ -98,7 +97,6 @@ abstract class AbstractSearchAsyncAction<Result extends SearchPhaseResult> exten
protected final GroupShardsIterator<SearchShardIterator> toSkipShardsIts;
protected final GroupShardsIterator<SearchShardIterator> shardsIts;
private final SearchShardIterator[] shardIterators;
private final Map<SearchShardIterator, Integer> shardIndexMap;
private final int expectedTotalOps;
private final AtomicInteger totalOps = new AtomicInteger();
private final int maxConcurrentRequestsPerNode;
Expand Down Expand Up @@ -142,17 +140,11 @@ abstract class AbstractSearchAsyncAction<Result extends SearchPhaseResult> exten
this.toSkipShardsIts = new GroupShardsIterator<>(toSkipIterators);
this.shardsIts = new GroupShardsIterator<>(iterators);

// we compute the shard index based on the natural order of the shards
this.shardIterators = iterators.toArray(new SearchShardIterator[0]);
// we later compute the shard index based on the natural order of the shards
// that participate in the search request. This means that this number is
// consistent between two requests that target the same shards.
Map<SearchShardIterator, Integer> shardMap = new HashMap<>();
List<SearchShardIterator> searchIterators = new ArrayList<>(iterators);
CollectionUtil.timSort(searchIterators);
for (int i = 0; i < searchIterators.size(); i++) {
shardMap.put(searchIterators.get(i), i);
}
this.shardIndexMap = Collections.unmodifiableMap(shardMap);
this.shardIterators = searchIterators.toArray(SearchShardIterator[]::new);
Arrays.sort(shardIterators);
Copy link
Member

Choose a reason for hiding this comment

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

can we remove the map while leaving the rest as it was?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure I was just annoyed by the needless extra copy and figured it's not worth an individual PR? :)

Copy link
Member

Choose a reason for hiding this comment

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

is timSort vs Arrays.sort on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we have a timSort for arrays in Lucene? + the Lucene timSort seems to be somewhat obsolete now anyway potentially with List#sort being a thing in recent Java versions and likely more efficient?
-> results are unchanged and this seems to be the canonical solution now :)?


// we need to add 1 for non active partition, since we count it in the total. This means for each shard in the iterator we sum up
// it's number of active shards but use 1 as the default if no replica of a shard is active at this point.
Expand Down Expand Up @@ -236,6 +228,10 @@ protected final void run() {
assert iterator.skip();
skipShard(iterator);
}
final Map<SearchShardIterator, Integer> shardIndexMap = Maps.newHashMapWithExpectedSize(shardIterators.length);
for (int i = 0; i < shardIterators.length; i++) {
shardIndexMap.put(shardIterators[i], i);
}
Copy link
Member

Choose a reason for hiding this comment

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

This is fine because run is always every called once on a specific async action instance?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, the results array among other things is pretty much single-use :)

if (shardsIts.size() > 0) {
doCheckNoMissingShards(getName(), request, shardsIts);
for (int i = 0; i < shardsIts.size(); i++) {
Expand Down