Make some Allocation Decider Code a Little More JIT Aware #62275

original-brownbear · 2020-09-11T20:56:34Z

When investigating our code cache usage for another issue I ran into this. This PR just fixes a few spots and there's many more. The current way we compute the decisions often creates much larger than necessary methods because the compiler has no efficient way of optimizing away things like using CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey() as an explain parameter that do allocations (but whose results are thrown away immediately if debug is off).

As a result e.g. the max retry allocation decider's canAllocate compiles into an 18kb method before and into a 2kb method after this change (at C1 L3). I think if we're a little more mindful of the JIT here we can get some measurable speedups out of the allocation deciders logic. Plus, this kind of change saves quite a few in allocations in isolation as well which is always nice on a hot CS thread I suppose.

elasticmachine · 2020-09-11T20:56:36Z

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

original-brownbear · 2020-09-12T13:12:10Z

Jenkins run elasticsearch-ci/bwc
Jenkins run elasticsearch-ci/default-distro

…-stuff

original-brownbear · 2020-09-13T20:43:54Z

Jenkins run elasticsearch-ci/packaging-sample-windows

…-stuff

henningandersen

Very interesting find @original-brownbear . I added a few initial comments, if nothing else to clarify my understanding of the issue.

henningandersen · 2020-09-14T10:55:38Z

.../org/elasticsearch/cluster/routing/allocation/decider/ClusterRebalanceAllocationDecider.java

+            case INDICES_PRIMARIES_ACTIVE:
+                // check if there are unassigned primaries.
+                if (routingNodes.hasUnassignedPrimaries()) {
+                    return debug ? NO_UNASSIGNED_PRIMARIES : Decision.NO;


Is there a reason we cannot just always return NO_UNASSIGNED_PRIMARIES? Looks like we can avoid the dependency on debug in this method.

++ that works as far as I can tell

henningandersen · 2020-09-14T10:58:07Z

.../org/elasticsearch/cluster/routing/allocation/decider/ClusterRebalanceAllocationDecider.java

+                if (routingNodes.hasInactiveShards()) {
+                    return debug ? NO_INACTIVE_SHARDS : Decision.NO;
+                }
        }


Add

// fall-through

to signal that fall through is intended.

henningandersen · 2020-09-14T10:58:46Z

.../org/elasticsearch/cluster/routing/allocation/decider/ClusterRebalanceAllocationDecider.java

        }
-        // type == Type.ALWAYS
-        return allocation.decision(Decision.YES, NAME, "all shards are active");
+        // all shards active from above or type == Type.ALWAYS


I wonder if this fits better into the switch now in a default block?

henningandersen · 2020-09-14T11:49:52Z

...ain/java/org/elasticsearch/cluster/routing/allocation/decider/MaxRetryAllocationDecider.java

-                    unassignedInfo.getNumFailedAllocations(), maxRetry);
-            }
+            final Decision res = numFailedAllocations >= maxRetry ? Decision.NO : Decision.YES;
+            decision = debug ? debugDecision(res, unassignedInfo, numFailedAllocations, maxRetry) : res;


Rather than inline the debug flag switch, would it be possible to use a supplier-style (perhaps a function, depending on input) just like is done for logging? So that it would be either:

allocation.decision(Decision.NO, NAME, "......[%d]...[%s]", a -> a.args(maxRetry, unassignedInfo.toString()))

or

allocation.decision(Decision.NO, NAME, res -> debugDecision(res, unassignedInfo, numFailedAllcations, maxRetry))

I guess technically yes, but it looks a lot more complicated and won't inline as well. I mean even for logging we often use
e.g. if (logger.isTraceEnabled()) {because the suppliers aren't free as well (especially when they capture a bunch of vars?).

henningandersen · 2020-09-14T11:50:44Z

...ain/java/org/elasticsearch/cluster/routing/allocation/decider/MaxRetryAllocationDecider.java

+        final boolean debug = allocation.debugDecision();
+        final int numFailedAllocations = unassignedInfo == null ? 0 : unassignedInfo.getNumFailedAllocations();
+        if (numFailedAllocations > 0) {
            final IndexMetadata indexMetadata = allocation.metadata().getIndexSafe(shardRouting.index());


I wonder if it was just as good (or better) to just extract the non-happy path here out into a method of its own?

Sure why not, certainly fits in with the theme of this PR :)

henningandersen · 2020-09-14T11:51:24Z

...ain/java/org/elasticsearch/cluster/routing/allocation/decider/MaxRetryAllocationDecider.java

+            decision = debug ? debugDecision(res, unassignedInfo, numFailedAllocations, maxRetry) : res;
        } else {
-            decision = allocation.decision(Decision.YES, NAME, "shard has no previous failures");
+            decision = debug ? YES_NO_FAILURES : Decision.YES;


I am not sure we need to switch on debug for purely constant decisions?

I looked into this and I think no we don't, we seem to only be using the full explanation in the explain allocation request -> will adjust accordingly

original-brownbear · 2020-09-14T13:26:36Z

@henningandersen thanks for taking a look! All points addressed I think :)

henningandersen

Left a few more comments.

henningandersen · 2020-09-18T13:42:47Z

.../org/elasticsearch/cluster/routing/allocation/decider/ClusterRebalanceAllocationDecider.java

        return canRebalance(allocation);
    }

+    private static final Decision YES_ALL_PRIMARIES_ACTIVE = Decision.single(Decision.Type.YES, NAME, "all primary shards are active");


I think it would be nice to add Decision.constant that still uses Decision.Single but avoids the trap of being able to specify parameters (or eagerly resolves the string if anyone do specify them).

Actually, I'm starting to wonder how much point there even is in making the String creation in the existing Decision.single lazy? The memory savings probably aren't that massive they only affect debug anyway?

++, seems just resolving this early is not a big deal. It will resolve it anyway in both equals, hashCode and streaming write.

Perfect :) Made it eager serialize now, also makes the Decision object immutable in general :)

henningandersen · 2020-09-18T13:45:07Z

...in/java/org/elasticsearch/cluster/routing/allocation/decider/SameShardAllocationDecider.java

            return decision;
        }
        if (node.node() != null) {
+            final boolean debug = allocation.debugDecision();


Maybe remove this variable that it is only used once?

henningandersen · 2020-09-18T14:04:04Z

.../org/elasticsearch/cluster/routing/allocation/decider/ClusterRebalanceAllocationDecider.java

+            case INDICES_PRIMARIES_ACTIVE:
+                // check if there are unassigned primaries.
+                if (routingNodes.hasUnassignedPrimaries()) {
+                    return NO_UNASSIGNED_PRIMARIES;


In AllocationDeciders we choose to early terminate no decisions, but only if the object is Decision.NO. I think we need to change that to check the underlying type if we return constants here.

Fixed by checking decision type :) thanks for spotting this!

I wonder if we can add a test that the early termination works in AllocationDeciders? At least one specific test with one specific example if making something that randomly exercises all decider NO decisions is too complicated.

…-stuff

original-brownbear · 2020-09-18T16:03:30Z

Thanks @henningandersen all points addressed I think :)

…-stuff

original-brownbear · 2020-10-29T10:17:47Z

@henningandersen ping here (low priority obviously) though I think it relates a to keeping making master nodes more responsive by burning less CPU + heap :)

henningandersen

LGTM.

henningandersen · 2020-10-30T11:35:58Z

...in/java/org/elasticsearch/cluster/routing/allocation/decider/AwarenessAllocationDecider.java

+    private static Decision debugNoMissingAttribute(String awarenessAttribute, List<String> awarenessAttributes) {
+        return Decision.single(Decision.Type.NO, NAME,
+                "node does not contain the awareness attribute [%s]; required attributes cluster setting ["
+                        + CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey() + "=%s]", awarenessAttribute,


Is there a reason for not using [%s] for the settings key?

I figured if we're already optimizing, why not just make this compile to one string constant instead of a constant format replacement

Sure, it just seems odd to have both forms (string concatenation and replacement) in the very same line?

Hmm replacement with a constant seems odd as well to me as well :) But I just realized that this is the debug path anyway, so I'm happy to change this back if you want.
That said, for better or for worse, we do have that pattern of mixing concatenation + replacement in a bunch of places for logging or for file name formatting in BlobstoreRepository for example?

OK, leave it as is, I can certainly gladly accept it as is, was a small nit only.

compile to one string constant

I think the getKey call prevents that? Though maybe the jit does something smart about this?

I think the getKey call prevents that? Though maybe the jit does something smart about this?

I would have thought the JIT can figure this out, but it took only a few minutes with JitWatch to learn that this is not the case. This initially compiles to:

private static org.elasticsearch.cluster.routing.allocation.decider.Decision debugNoMissingAttribute(java.lang.String, java.util.List<java.lang.String>); Code: 0: getstatic #236 // Field org/elasticsearch/cluster/routing/allocation/decider/Decision$Type.NO:Lorg/elasticsearch/cluster/routing/allocation/decider/Decision$Type; 3: ldc #241 // String awareness 5: getstatic #7 // Field CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING:Lorg/elasticsearch/common/settings/Setting; 8: invokevirtual #257 // Method org/elasticsearch/common/settings/Setting.getKey:()Ljava/lang/String; 11: invokedynamic #259, 0 // InvokeDynamic #2:makeConcatWithConstants:(Ljava/lang/String;)Ljava/lang/String; 16: iconst_2 17: anewarray #245 // class java/lang/Object 20: dup 21: iconst_0 22: aload_0 23: aastore 24: dup 25: iconst_1 26: aload_1

and all that happens is that the getKey call is eventually inlined but the string concatenation still happens every time.

-> I'll revert this before merging in a bit :)

henningandersen · 2020-10-30T11:41:22Z

.../org/elasticsearch/cluster/routing/allocation/decider/ClusterRebalanceAllocationDecider.java

+            case INDICES_PRIMARIES_ACTIVE:
+                // check if there are unassigned primaries.
+                if (routingNodes.hasUnassignedPrimaries()) {
+                    return NO_UNASSIGNED_PRIMARIES;


I wonder if we can add a test that the early termination works in AllocationDeciders? At least one specific test with one specific example if making something that randomly exercises all decider NO decisions is too complicated.

original-brownbear · 2020-10-30T13:11:28Z

I wonder if we can add a test that the early termination works in AllocationDeciders?

Looking into that :)

…me-expensive-stuff

original-brownbear · 2020-10-30T14:31:29Z

@henningandersen I pushed 7813daf for the test of the short-circuit logic Maybe take another look since I also slightly changed the returns of the methods in AllocationDeciders so that we still always return a plain Decision.NO from those in the non-debug case?
Thanks!

henningandersen

LGTM.

original-brownbear · 2020-10-30T17:30:14Z

Thanks Henning!

) When investigating our code cache usage for another issue I ran into this. This PR just fixes a few spots and there's many more. The current way we compute the decisions often creates much larger than necessary methods because the compiler has no efficient way of optimizing away things like using CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey() as an explain parameter that do allocations (but whose results are thrown away immediately if debug is off). As a result e.g. the max retry allocation decider's canAllocate compiles into an 18kb method before and into a 2kb method after this change (at C1 L3). I think if we're a little more mindful of the JIT here we can get some measurable speedups out of the allocation deciders logic. Plus, this kind of change saves quite a few in allocations in isolation as well which is always nice on a hot CS thread I suppose.

…64444) When investigating our code cache usage for another issue I ran into this. This PR just fixes a few spots and there's many more. The current way we compute the decisions often creates much larger than necessary methods because the compiler has no efficient way of optimizing away things like using CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey() as an explain parameter that do allocations (but whose results are thrown away immediately if debug is off). As a result e.g. the max retry allocation decider's canAllocate compiles into an 18kb method before and into a 2kb method after this change (at C1 L3). I think if we're a little more mindful of the JIT here we can get some measurable speedups out of the allocation deciders logic. Plus, this kind of change saves quite a few in allocations in isolation as well which is always nice on a hot CS thread I suppose.

Some smaller improvements in the direction of elastic#62275 and removal of some dead code and duplication.

Some smaller improvements in the direction of #62275 and removal of some dead code and duplication.

Some smaller improvements in the direction of elastic#62275 and removal of some dead code and duplication.

Some smaller improvements in the direction of #62275 and removal of some dead code and duplication.

In elastic#62275 we refactored this code a bit and inadvertently reversed the sense of this conditional when running in debug mode. This commit fixes the mistake.

In #62275 we refactored this code a bit and inadvertently reversed the sense of this conditional when running in debug mode. This commit fixes the mistake.

In elastic#62275 we refactored this code a bit and inadvertently reversed the sense of this conditional when running in debug mode. This commit fixes the mistake.

In #62275 we refactored this code a bit and inadvertently reversed the sense of this conditional when running in debug mode. This commit fixes the mistake.

In #62275 we refactored this code a bit and inadvertently reversed the sense of this conditional when running in debug mode. This commit fixes the mistake. Co-authored-by: Elastic Machine <[email protected]>

much nicer

e53fbf0

original-brownbear added >non-issue :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) v8.0.0 v7.10.0 labels Sep 11, 2020

elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Sep 11, 2020

original-brownbear changed the title ~~Make Allocation Decider Code a Little more JIT Aware~~ Make Some Allocation Decider Code a Little More JIT Aware Sep 11, 2020

original-brownbear changed the title ~~Make Some Allocation Decider Code a Little More JIT Aware~~ Make some Allocation Decider Code a Little More JIT Aware Sep 11, 2020

Merge remote-tracking branch 'elastic/master' into fix-some-expensive…

ad98bd5

…-stuff

Merge remote-tracking branch 'elastic/master' into fix-some-expensive…

99019b3

…-stuff

original-brownbear requested review from DaveCTurner and henningandersen September 14, 2020 09:48

henningandersen reviewed Sep 14, 2020

View reviewed changes

CR comments

6e772a0

original-brownbear requested a review from henningandersen September 14, 2020 13:26

henningandersen reviewed Sep 18, 2020

View reviewed changes

original-brownbear added 3 commits September 18, 2020 17:03

Merge remote-tracking branch 'elastic/master' into fix-some-expensive…

7ef2fc7

…-stuff

remove redundant var + improve no check

1f811c8

no more layy explain string

6bd6608

original-brownbear requested a review from henningandersen September 18, 2020 16:03

andreidan added v7.11.0 and removed v7.10.0 labels Oct 7, 2020

Merge remote-tracking branch 'elastic/master' into fix-some-expensive…

8a59791

…-stuff

henningandersen approved these changes Oct 30, 2020

View reviewed changes

original-brownbear added 2 commits October 30, 2020 14:17

Merge branch 'master' of github.com:elastic/elasticsearch into fix-so…

89bc2c6

…me-expensive-stuff

CR: add test for short-circuit

7813daf

original-brownbear requested a review from henningandersen October 30, 2020 14:31

henningandersen approved these changes Oct 30, 2020

View reviewed changes

replace all the way

765850c

original-brownbear merged commit 0ed5174 into elastic:master Oct 30, 2020

original-brownbear deleted the fix-some-expensive-stuff branch October 30, 2020 17:30

original-brownbear added backport pending and removed backport pending labels Oct 30, 2020

original-brownbear mentioned this pull request Oct 30, 2020

Make some Allocation Decider Code a Little More JIT Aware (#62275) #64444

Merged

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Nov 6, 2020

Small Simplifications DiskThresholdDecider

905c2c2

Some smaller improvements in the direction of elastic#62275 and removal of some dead code and duplication.

original-brownbear mentioned this pull request Nov 6, 2020

Small Simplifications DiskThresholdDecider #64703

Merged

original-brownbear added a commit that referenced this pull request Nov 6, 2020

Small Simplifications DiskThresholdDecider (#64703)

efecaed

Some smaller improvements in the direction of #62275 and removal of some dead code and duplication.

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Nov 6, 2020

Small Simplifications DiskThresholdDecider (elastic#64703)

3af536a

Some smaller improvements in the direction of elastic#62275 and removal of some dead code and duplication.

original-brownbear mentioned this pull request Nov 6, 2020

Small Simplifications DiskThresholdDecider (#64703) #64719

Merged

original-brownbear added a commit that referenced this pull request Nov 6, 2020

Small Simplifications DiskThresholdDecider (#64703) (#64719)

0c0c756

Some smaller improvements in the direction of #62275 and removal of some dead code and duplication.

original-brownbear restored the fix-some-expensive-stuff branch December 6, 2020 19:01

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

DaveCTurner mentioned this pull request Sep 9, 2022

Fix debug mode in MaxRetryAllocationDecider #89973

Merged

DaveCTurner added a commit that referenced this pull request Sep 9, 2022

Fix debug mode in MaxRetryAllocationDecider (#89973)

cb40ab8

In #62275 we refactored this code a bit and inadvertently reversed the sense of this conditional when running in debug mode. This commit fixes the mistake.

Make some Allocation Decider Code a Little More JIT Aware #62275

Make some Allocation Decider Code a Little More JIT Aware #62275

Uh oh!

Conversation

original-brownbear commented Sep 11, 2020

Uh oh!

elasticmachine commented Sep 11, 2020

Uh oh!

original-brownbear commented Sep 12, 2020

Uh oh!

original-brownbear commented Sep 13, 2020

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!

original-brownbear commented Sep 14, 2020

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!

original-brownbear commented Sep 18, 2020

Uh oh!

original-brownbear commented Oct 29, 2020

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