Save some RoutingNodes Instantiations by original-brownbear · Pull Request #79941 · elastic/elasticsearch

original-brownbear · 2021-10-27T17:42:01Z

This commit introduces the ability to create a mutable copy of RoutingNodes
instead of having to re-compute the data structures from scratch which is much
faster and allows for some reuse of immutable parts of a routing nodes instance.

Also, this sets up further improvements that avoid some more redundant routing node
instantiations in RoutingAllocation.

relates #77466

This commit introduces the ability to create a mutable copy of `RoutingNodes` instead of having to re-compute the data structures from scratch which is much faster and allows for some reuse of immutable parts of a routing nodes instance. Also, this sets up further improvements that avoid some more redundant routing node instantiations in `RoutingAllocation`.

elasticmachine · 2021-10-27T17:42:05Z

Pinging @elastic/es-distributed (Team:Distributed)

original-brownbear · 2021-10-27T18:12:00Z

@elasticmachine update branch

henningandersen

I left a few smaller comments. My main question is about the benefits of this and whether we are actually saving any significant amounts of instantiation by doing this. Do you have some data on that?

One option I was wondering about is whether the normal RoutingNodes constructor can simply be tuned enough to have equivalent performance, but it might not be viable. Did you examine that direction?

henningandersen · 2021-11-02T08:15:51Z

server/src/main/java/org/elasticsearch/cluster/ClusterState.java

+            return true;
+        }
+        final RoutingNodes expected = new RoutingNodes(routingTable, nodes);
+        assert routingNodes.equals(expected)


equals does not look like it compares all fields, for instance on ShardRouting. Also, it would be even nicer to verify that all the ShardRouting instances are the same?

But then again, that really does not change with this PR, so feel free to disregard this.

henningandersen · 2021-11-02T08:23:33Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

+        assert invariant();
+    }
+
+    public RoutingNode copy() {


Can this be package private instead?

Also, perhaps rename to mutableCopy?

Sure, though these instances are all mutable to begin with, so maybe leave the name? :)

henningandersen · 2021-11-02T08:34:42Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNodes.java

+            this.ignoredPrimaries = ignoredPrimaries;
+        }
+
+        public UnassignedShards copyTo(RoutingNodes newNodes) {


Perhaps rename to copyFor, at least to me To signals that it writes into the argument.

henningandersen · 2021-11-02T08:37:20Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java

+            if (version == 0 && indicesRouting.isEmpty()) {
+                table = EMPTY_ROUTING_TABLE;


I am not sure I understand when this helps us, can you elaborate and/or comment?

Hmm this just leaked in here from the other branch. It doesn't really give a big performance boost, it's more of a cleanup and I suppose saves a couple of cycles around this code in tests :) I can back it out if it's too noisy?

+ε to revert

reverted :)

henningandersen · 2021-11-02T08:39:30Z

...est/java/org/elasticsearch/cluster/routing/allocation/decider/DiskThresholdDeciderTests.java

        RoutingAllocation routingAllocation = new RoutingAllocation(
            null,
-            new RoutingNodes(clusterState),
+            new RoutingNodes(clusterState.routingTable(), clusterState.nodes()),


Would clusterState.getRoutingNodes() not work here too (and in the other tests using the constructor)?

Yes, literally the only reason I didn't use it was because that would instantiate that field on the ClusterState and thus change the behavior of the test. I can't see how that would be relevant here in practice, but I didn't want to make any changes to this in this test (could have just called that method before as well).

henningandersen · 2021-11-02T08:42:36Z

...st/java/org/elasticsearch/xpack/ccr/allocation/CcrPrimaryFollowerAllocationDeciderTests.java

        final RoutingAllocation routingAllocation = new RoutingAllocation(
            new AllocationDeciders(List.of(decider)),
-            new RoutingNodes(clusterState),
+            new RoutingNodes(clusterState.routingTable(), clusterState.nodes()),


Can we use clusterState.getRoutingNodes() instead?

Same answer as above, probably fine to make that change but I didn't want to change test behavior here.

henningandersen · 2021-11-02T08:44:40Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNodes.java

+    public RoutingNodes(RoutingTable routingTable, DiscoveryNodes discoveryNodes) {
+        this(routingTable, discoveryNodes, true);
    }

-    public RoutingNodes(ClusterState clusterState, boolean readOnly) {
+    public RoutingNodes(RoutingTable routingTable, DiscoveryNodes discoveryNodes, boolean readOnly) {


I wonder if we should make these private and add factory methods with names signaling the read-only-ness as well as that they are only to be called by ClusterState.

Sure moved to two separate factory methods here, looks much cleaner indeed.

…g-temporary-routing-nodes

original-brownbear · 2021-11-02T09:04:14Z

My main question is about the benefits of this and whether we are actually saving any significant amounts of instantiation by doing this. Do you have some data on that?

Yes, this is one of the most expensive steps in re-route today (and becomes the most expensive step in it once other fixes in #77466 to index settings have made it in). This makes the part where we create mutable routing nodes more than 50% cheaper in many cases which is a 10% saving on the full operation even today (and a larger one relatively speaking once the other fixes hit).
Note, that even though instantiating routing nodes is lazy in theory in practice we pretty much always instantiate it for every cluster state so we always get the savings from creating a mutable copy instead of rebuilding the full data structure pretty much. With all fixes from #77466 that fix all spots where we are not re-using a routing nodes instance that I could find that practically becomes 100% of reroutes that see the saving.

One option I was wondering about is whether the normal RoutingNodes constructor can simply be tuned enough to have equivalent performance, but it might not be viable. Did you examine that direction?

I have and #77466 contains a number of improvements on that front as well, there's limits on how far optimizations can go here though without fundamentally changing things.

…uting-nodes' into avoid-instantiating-temporary-routing-nodes

DaveCTurner

LGTM.

We should probably have different classes for mutable and immutable RoutingNodes, given how many of the methods don't make sense if readOnly is set.

DaveCTurner · 2021-12-06T11:03:05Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java

+            if (version == 0 && indicesRouting.isEmpty()) {
+                table = EMPTY_ROUTING_TABLE;


+ε to revert

DaveCTurner · 2021-12-06T11:10:57Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNodes.java

+        this.relocatingShards = routingNodes.relocatingShards;
+        this.activeShardCount = routingNodes.activeShardCount;
+        this.totalShardCount = routingNodes.totalShardCount;
+        this.attributeValuesByAttribute = new HashMap<>(routingNodes.attributeValuesByAttribute.size());


This is a lazy cache of some immutable properties of the immutable set of nodes so it's never meaningfully mutated, so really we could just re-use the same map, or else instantiate it as an empty map. Also if it's only used in AwarenessAllocationDecider it should be empty on immutable instances anyway. But maybe it's nicer to explicitly copy it here anyway just for clarity.

Maybe we should move this to DiscoveryNodes so it doesn't get invalidated on every cluster state update too. Probably not a big deal, just thinking out loud here.

DaveCTurner · 2021-12-06T11:11:43Z

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNodes.java

+    public RoutingNodes mutableCopy() {
+        // we should not call this on mutable instances, it's still expensive to create the copy and callers should instead mutate a single
+        // instance
+        assert readOnly : "tried to create a mutable copy from a mutable instance";


Could we assert this in the (private) constructor instead, just in case we inadvertently add another caller?

…g-temporary-routing-nodes

original-brownbear · 2021-12-13T13:21:16Z

Jenkins run elasticsearch-ci/part-1 (known failure)

original-brownbear · 2021-12-13T13:55:50Z

Thanks David & Henning!

original-brownbear added >non-issue :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) v8.0.0 v8.1.0 labels Oct 27, 2021

elasticmachine added the Team:Distributed Meta label for distributed team. label Oct 27, 2021

original-brownbear mentioned this pull request Oct 27, 2021

Fix Large Shard Count Scalability Issues #77466

Open

97 tasks

static'

eaafcdb

Merge branch 'master' into avoid-instantiating-temporary-routing-nodes

8708e64

original-brownbear requested review from DaveCTurner and henningandersen October 28, 2021 09:36

henningandersen reviewed Nov 2, 2021

View reviewed changes

Merge remote-tracking branch 'elastic/master' into avoid-instantiatin…

e4aaad7

…g-temporary-routing-nodes

original-brownbear added 4 commits November 2, 2021 10:09

CR: comments

b403853

more comments

dda2cdd

Merge remote-tracking branch 'origin/avoid-instantiating-temporary-ro…

0c7738e

…uting-nodes' into avoid-instantiating-temporary-routing-nodes

docs

7314823

original-brownbear requested a review from henningandersen November 2, 2021 10:45

DaveCTurner approved these changes Dec 6, 2021

View reviewed changes

original-brownbear added 2 commits December 13, 2021 12:59

Merge remote-tracking branch 'elastic/master' into avoid-instantiatin…

71a898f

…g-temporary-routing-nodes

move assertion + revert singleton use

4288040

original-brownbear removed the v8.0.0 label Dec 13, 2021

original-brownbear merged commit a5e3c6d into elastic:master Dec 13, 2021

original-brownbear deleted the avoid-instantiating-temporary-routing-nodes branch December 13, 2021 13:56

original-brownbear restored the avoid-instantiating-temporary-routing-nodes branch April 18, 2023 20:59

		if (version == 0 && indicesRouting.isEmpty()) {
		table = EMPTY_ROUTING_TABLE;

Conversation

original-brownbear commented Oct 27, 2021

Uh oh!

elasticmachine commented Oct 27, 2021

Uh oh!

original-brownbear commented Oct 27, 2021

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Nov 2, 2021

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Dec 13, 2021

Uh oh!

original-brownbear commented Dec 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants