Skip to content

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

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 11 commits into from
Oct 30, 2020
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e53fbf0
much nicer
original-brownbear Sep 11, 2020
ad98bd5
Merge remote-tracking branch 'elastic/master' into fix-some-expensive…
original-brownbear Sep 13, 2020
99019b3
Merge remote-tracking branch 'elastic/master' into fix-some-expensive…
original-brownbear Sep 14, 2020
6e772a0
CR comments
original-brownbear Sep 14, 2020
7ef2fc7
Merge remote-tracking branch 'elastic/master' into fix-some-expensive…
original-brownbear Sep 18, 2020
1f811c8
remove redundant var + improve no check
original-brownbear Sep 18, 2020
6bd6608
no more layy explain string
original-brownbear Sep 18, 2020
8a59791
Merge remote-tracking branch 'elastic/master' into fix-some-expensive…
original-brownbear Oct 29, 2020
89bc2c6
Merge branch 'master' of github.com:elastic/elasticsearch into fix-so…
original-brownbear Oct 30, 2020
7813daf
CR: add test for short-circuit
original-brownbear Oct 30, 2020
765850c
replace all the way
original-brownbear Oct 30, 2020
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
50 changes: 33 additions & 17 deletions ...java/org/elasticsearch/cluster/routing/allocation/decider/AwarenessAllocationDecider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,22 +125,25 @@ public Decision canRemain(ShardRouting shardRouting, RoutingNode node, RoutingAl
return underCapacity(shardRouting, node, allocation, false);
}

private static final Decision YES_NOT_ENABLED = Decision.single(Decision.Type.YES, NAME,
"allocation awareness is not enabled, set cluster setting ["
+ CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey() + "] to enable it");

private static final Decision YES_ALL_MET =
Decision.single(Decision.Type.YES, NAME, "node meets all awareness attribute requirements");

private Decision underCapacity(ShardRouting shardRouting, RoutingNode node, RoutingAllocation allocation, boolean moveToNode) {
final boolean debug = allocation.debugDecision();
if (awarenessAttributes.isEmpty()) {
return allocation.decision(Decision.YES, NAME,
"allocation awareness is not enabled, set cluster setting [%s] to enable it",
CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey());
return debug ? YES_NOT_ENABLED : Decision.YES;
}

IndexMetadata indexMetadata = allocation.metadata().getIndexSafe(shardRouting.index());
int shardCount = indexMetadata.getNumberOfReplicas() + 1; // 1 for primary
for (String awarenessAttribute : awarenessAttributes) {
// the node the shard exists on must be associated with an awareness attribute
if (node.node().getAttributes().containsKey(awarenessAttribute) == false) {
return allocation.decision(Decision.NO, NAME,
"node does not contain the awareness attribute [%s]; required attributes cluster setting [%s=%s]",
awarenessAttribute, CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey(),
allocation.debugDecision() ? Strings.collectionToCommaDelimitedString(awarenessAttributes) : null);
return debug ? debugNoMissingAttribute(awarenessAttribute, awarenessAttributes) : Decision.NO;
}

// build attr_value -> nodes map
Expand Down Expand Up @@ -185,18 +188,31 @@ private Decision underCapacity(ShardRouting shardRouting, RoutingNode node, Rout
final int currentNodeCount = shardPerAttribute.get(node.node().getAttributes().get(awarenessAttribute));
final int maximumNodeCount = (shardCount + numberOfAttributes - 1) / numberOfAttributes; // ceil(shardCount/numberOfAttributes)
if (currentNodeCount > maximumNodeCount) {
return allocation.decision(Decision.NO, NAME,
"there are too many copies of the shard allocated to nodes with attribute [%s], there are [%d] total configured " +
"shard copies for this shard id and [%d] total attribute values, expected the allocated shard count per " +
"attribute [%d] to be less than or equal to the upper bound of the required number of shards per attribute [%d]",
awarenessAttribute,
shardCount,
numberOfAttributes,
currentNodeCount,
maximumNodeCount);
return debug ? debugNoTooManyCopies(shardCount, awarenessAttribute, numberOfAttributes, currentNodeCount, maximumNodeCount)
: Decision.NO;
}
}

return allocation.decision(Decision.YES, NAME, "node meets all awareness attribute requirements");
return debug ? YES_ALL_MET : Decision.YES;
}

private static Decision debugNoTooManyCopies(int shardCount, String awarenessAttribute, int numberOfAttributes, int currentNodeCount,
int maximumNodeCount) {
return Decision.single(Decision.Type.NO, NAME,
"there are too many copies of the shard allocated to nodes with attribute [%s], there are [%d] total configured " +
"shard copies for this shard id and [%d] total attribute values, expected the allocated shard count per " +
"attribute [%d] to be less than or equal to the upper bound of the required number of shards per attribute [%d]",
awarenessAttribute,
shardCount,
numberOfAttributes,
currentNodeCount,
maximumNodeCount);
}

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,
Copy link
Contributor

@henningandersen henningandersen Oct 30, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@original-brownbear original-brownbear Oct 30, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@henningandersen henningandersen Oct 30, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@original-brownbear original-brownbear Oct 30, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@henningandersen henningandersen Oct 30, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@original-brownbear original-brownbear Oct 30, 2020

Choose a reason for hiding this comment

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

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

Strings.collectionToCommaDelimitedString(awarenessAttributes));
}
}
78 changes: 46 additions & 32 deletions ...g/elasticsearch/cluster/routing/allocation/decider/ClusterRebalanceAllocationDecider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.elasticsearch.cluster.routing.RoutingNodes;
import org.elasticsearch.cluster.routing.ShardRouting;
import org.elasticsearch.cluster.routing.allocation.RoutingAllocation;
import org.elasticsearch.common.settings.ClusterSettings;
Expand Down Expand Up @@ -109,40 +110,53 @@ public Decision canRebalance(ShardRouting shardRouting, RoutingAllocation alloca
return canRebalance(allocation);
}

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

@henningandersen henningandersen Sep 18, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@original-brownbear original-brownbear Sep 18, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@henningandersen henningandersen Sep 18, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@original-brownbear original-brownbear Sep 18, 2020

Choose a reason for hiding this comment

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

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


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

private static final Decision NO_UNASSIGNED_PRIMARIES = Decision.single(Decision.Type.NO, NAME,
"the cluster has unassigned primary shards and cluster setting ["
+ CLUSTER_ROUTING_ALLOCATION_ALLOW_REBALANCE + "] is set to [" + ClusterRebalanceType.INDICES_PRIMARIES_ACTIVE + "]");

private static final Decision NO_INACTIVE_PRIMARIES = Decision.single(Decision.Type.NO, NAME,
"the cluster has inactive primary shards and cluster setting [" + CLUSTER_ROUTING_ALLOCATION_ALLOW_REBALANCE +
"] is set to [" + ClusterRebalanceType.INDICES_PRIMARIES_ACTIVE + "]");

private static final Decision NO_UNASSIGNED_SHARDS = Decision.single(Decision.Type.NO, NAME,
"the cluster has unassigned shards and cluster setting [" + CLUSTER_ROUTING_ALLOCATION_ALLOW_REBALANCE +
"] is set to [" + ClusterRebalanceType.INDICES_ALL_ACTIVE + "]");

private static final Decision NO_INACTIVE_SHARDS = Decision.single(Decision.Type.NO, NAME,
"the cluster has inactive shards and cluster setting [" + CLUSTER_ROUTING_ALLOCATION_ALLOW_REBALANCE +
"] is set to [" + ClusterRebalanceType.INDICES_ALL_ACTIVE + "]");

@Override
public Decision canRebalance(RoutingAllocation allocation) {
if (type == ClusterRebalanceType.INDICES_PRIMARIES_ACTIVE) {
// check if there are unassigned primaries.
if ( allocation.routingNodes().hasUnassignedPrimaries() ) {
return allocation.decision(Decision.NO, NAME,
"the cluster has unassigned primary shards and cluster setting [%s] is set to [%s]",
CLUSTER_ROUTING_ALLOCATION_ALLOW_REBALANCE, type);
}
// check if there are initializing primaries that don't have a relocatingNodeId entry.
if ( allocation.routingNodes().hasInactivePrimaries() ) {
return allocation.decision(Decision.NO, NAME,
"the cluster has inactive primary shards and cluster setting [%s] is set to [%s]",
CLUSTER_ROUTING_ALLOCATION_ALLOW_REBALANCE, type);
}

return allocation.decision(Decision.YES, NAME, "all primary shards are active");
}
if (type == ClusterRebalanceType.INDICES_ALL_ACTIVE) {
// check if there are unassigned shards.
if (allocation.routingNodes().hasUnassignedShards() ) {
return allocation.decision(Decision.NO, NAME,
"the cluster has unassigned shards and cluster setting [%s] is set to [%s]",
CLUSTER_ROUTING_ALLOCATION_ALLOW_REBALANCE, type);
}
// in case all indices are assigned, are there initializing shards which
// are not relocating?
if ( allocation.routingNodes().hasInactiveShards() ) {
return allocation.decision(Decision.NO, NAME,
"the cluster has inactive shards and cluster setting [%s] is set to [%s]",
CLUSTER_ROUTING_ALLOCATION_ALLOW_REBALANCE, type);
}
final boolean debug = allocation.debugDecision();
final RoutingNodes routingNodes = allocation.routingNodes();
switch (type) {
case INDICES_PRIMARIES_ACTIVE:
// check if there are unassigned primaries.
if (routingNodes.hasUnassignedPrimaries()) {
return debug ? NO_UNASSIGNED_PRIMARIES : Decision.NO;
Copy link
Contributor

@henningandersen henningandersen Sep 14, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@original-brownbear original-brownbear Sep 14, 2020

Choose a reason for hiding this comment

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

++ that works as far as I can tell

}
// check if there are initializing primaries that don't have a relocatingNodeId entry.
if (routingNodes.hasInactivePrimaries()) {
return debug ? NO_INACTIVE_PRIMARIES : Decision.NO;
}
return debug ? YES_ALL_PRIMARIES_ACTIVE : Decision.YES;
case INDICES_ALL_ACTIVE:
// check if there are unassigned shards.
if (routingNodes.hasUnassignedShards()) {
return debug ? NO_UNASSIGNED_SHARDS : Decision.NO;
}
// in case all indices are assigned, are there initializing shards which
// are not relocating?
if (routingNodes.hasInactiveShards()) {
return debug ? NO_INACTIVE_SHARDS : Decision.NO;
}
}
Copy link
Contributor

@henningandersen henningandersen Sep 14, 2020

Choose a reason for hiding this comment

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

Add

// fall-through

to signal that fall through is intended.

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

@henningandersen henningandersen Sep 14, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@original-brownbear original-brownbear Sep 14, 2020

Choose a reason for hiding this comment

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

++

return debug ? YES_ALL_SHARDS_ACTIVE : Decision.YES;
}
}
38 changes: 24 additions & 14 deletions server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/Decision.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,20 @@ public static Decision readFrom(StreamInput in) throws IOException {
}
return result;
} else {
Single result = new Single();
result.type = Type.readFrom(in);
result.label = in.readOptionalString();
result.explanationString = in.readOptionalString();
return result;
final Type type = Type.readFrom(in);
final String label = in.readOptionalString();
final String explanation = in.readOptionalString();
if (label == null && explanation == null) {
switch (type) {
case YES:
return YES;
case THROTTLE:
return THROTTLE;
case NO:
return NO;
}
}
return new Single(type, label, explanation);
}
}

Expand Down Expand Up @@ -153,21 +162,17 @@ public boolean higherThan(Type other) {
* Simple class representing a single decision
*/
public static class Single extends Decision implements ToXContentObject {
private Type type;
private String label;
private String explanation;
private final Type type;
private final String label;
private final String explanation;
private String explanationString;
private Object[] explanationParams;

public Single() {

}
private final Object[] explanationParams;

/**
* Creates a new {@link Single} decision of a given type
* @param type {@link Type} of the decision
*/
public Single(Type type) {
private Single(Type type) {
this(type, null, null, (Object[]) null);
}

Expand All @@ -183,6 +188,11 @@ public Single(Type type, @Nullable String label, @Nullable String explanation, @
this.label = label;
this.explanation = explanation;
this.explanationParams = explanationParams;
if (explanationParams == null || explanationParams.length == 0) {
// If no formatting is required assign this right away so that we don't needlessly run into any races when using this class
// for constants
this.explanationString = explanation;
}
}

@Override
Expand Down
29 changes: 19 additions & 10 deletions .../java/org/elasticsearch/cluster/routing/allocation/decider/MaxRetryAllocationDecider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,27 +41,36 @@ public class MaxRetryAllocationDecider extends AllocationDecider {

public static final String NAME = "max_retry";

private static final Decision YES_NO_FAILURES = Decision.single(Decision.Type.YES, NAME, "shard has no previous failures");

@Override
public Decision canAllocate(ShardRouting shardRouting, RoutingAllocation allocation) {
final UnassignedInfo unassignedInfo = shardRouting.unassignedInfo();
final Decision decision;
if (unassignedInfo != null && unassignedInfo.getNumFailedAllocations() > 0) {
final boolean debug = allocation.debugDecision();
final int numFailedAllocations = unassignedInfo == null ? 0 : unassignedInfo.getNumFailedAllocations();
if (numFailedAllocations > 0) {
final IndexMetadata indexMetadata = allocation.metadata().getIndexSafe(shardRouting.index());
Copy link
Contributor

@henningandersen henningandersen Sep 14, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@original-brownbear original-brownbear Sep 14, 2020

Choose a reason for hiding this comment

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

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

final int maxRetry = SETTING_ALLOCATION_MAX_RETRY.get(indexMetadata.getSettings());
if (unassignedInfo.getNumFailedAllocations() >= maxRetry) {
decision = allocation.decision(Decision.NO, NAME, "shard has exceeded the maximum number of retries [%d] on " +
"failed allocation attempts - manually call [/_cluster/reroute?retry_failed=true] to retry, [%s]",
maxRetry, unassignedInfo.toString());
} else {
decision = allocation.decision(Decision.YES, NAME, "shard has failed allocating [%d] times but [%d] retries are allowed",
unassignedInfo.getNumFailedAllocations(), maxRetry);
}
final Decision res = numFailedAllocations >= maxRetry ? Decision.NO : Decision.YES;
decision = debug ? debugDecision(res, unassignedInfo, numFailedAllocations, maxRetry) : res;
Copy link
Contributor

@henningandersen henningandersen Sep 14, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@original-brownbear original-brownbear Sep 14, 2020

Choose a reason for hiding this comment

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

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?).

} else {
decision = allocation.decision(Decision.YES, NAME, "shard has no previous failures");
decision = debug ? YES_NO_FAILURES : Decision.YES;
Copy link
Contributor

@henningandersen henningandersen Sep 14, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@original-brownbear original-brownbear Sep 14, 2020

Choose a reason for hiding this comment

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

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

}
return decision;
}

private static Decision debugDecision(Decision decision, UnassignedInfo unassignedInfo, int numFailedAllocations, int maxRetry) {
if (decision.type() == Decision.Type.YES) {
return Decision.single(Decision.Type.NO, NAME, "shard has exceeded the maximum number of retries [%d] on " +
"failed allocation attempts - manually call [/_cluster/reroute?retry_failed=true] to retry, [%s]",
maxRetry, unassignedInfo.toString());
} else {
return Decision.single(Decision.Type.YES, NAME, "shard has failed allocating [%d] times but [%d] retries are allowed",
numFailedAllocations, maxRetry);
}
}

@Override
public Decision canAllocate(ShardRouting shardRouting, RoutingNode node, RoutingAllocation allocation) {
return canAllocate(shardRouting, allocation);
Expand Down
Loading