Skip to content

Replace IndexNameExpressionResolver.ExpressionList with imperative logic #115487

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 3 commits into from
Oct 24, 2024
Merged

Replace IndexNameExpressionResolver.ExpressionList with imperative logic #115487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50f4a01
Replace IndexNameExpressionResolver.ExpressionList with imperative logic
original-brownbear Oct 24, 2024
3c01201
Merge remote-tracking branch 'elastic/main' into drop-expression-list
original-brownbear Oct 24, 2024
6466118
doc
original-brownbear Oct 24, 2024
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
186 changes: 85 additions & 101 deletions server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -253,7 +252,7 @@ protected static Collection<String> resolveExpressions(Context context, String..
} else {
return ExplicitResourceNameFilter.filterUnavailable(
context,
DateMathExpressionResolver.resolve(context, List.of(expressions))
DateMathExpressionResolver.resolve(context, Arrays.asList(expressions))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a pretty big deal in some cases, List.of will copy the expressions list which is quite heavy once we get to very long lists.

Copy link
Member

Choose a reason for hiding this comment

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

With this, I think we moved from a defensive copy of the expression array into a mutable view. Are there any concerns about later code using .set(…) on this list and potentially mutating the expressions array? Is that a danger we're willing to accept? (For what it's worth, I don't see any instances of .set(…) in the code here that is mutating the expressions list, so we may just want to document not to modify it for the future).

Copy link
Contributor

Choose a reason for hiding this comment

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

You could Collections.unmodifiableList(...) the list from Arrays.asList if you wanted. It's an allocation and adds some overhead to the list operations, but I don't think it's nearly as much work as copying the entire list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually thinking about dropping the Arrays wrapping in a follow-up :P that would make this perform yet a little nicer actually :D

);
}
} else {
Expand All @@ -264,7 +263,10 @@ protected static Collection<String> resolveExpressions(Context context, String..
} else {
return WildcardExpressionResolver.resolve(
context,
ExplicitResourceNameFilter.filterUnavailable(context, DateMathExpressionResolver.resolve(context, List.of(expressions)))
ExplicitResourceNameFilter.filterUnavailable(
context,
DateMathExpressionResolver.resolve(context, Arrays.asList(expressions))
)
);
}
}
Expand Down Expand Up @@ -1294,34 +1296,51 @@ private static boolean shouldIncludeIfAlias(IndexAbstraction ia, IndexNameExpres
* </ol>
*/
public static Collection<String> resolve(Context context, List<String> expressions) {
ExpressionList expressionList = new ExpressionList(context, expressions);
// fast exit if there are no wildcards to evaluate
if (expressionList.hasWildcard() == false) {
if (context.getOptions().expandWildcardExpressions() == false) {
return expressions;
}
int firstWildcardIndex = 0;
for (; firstWildcardIndex < expressions.size(); firstWildcardIndex++) {
String expression = expressions.get(firstWildcardIndex);
if (isWildcard(expression)) {
break;
}
}
if (firstWildcardIndex == expressions.size()) {
return expressions;
}
Set<String> result = new HashSet<>();
for (ExpressionList.Expression expression : expressionList) {
if (expression.isWildcard()) {
Stream<IndexAbstraction> matchingResources = matchResourcesToWildcard(context, expression.get());
for (int i = 0; i < firstWildcardIndex; i++) {
result.add(expressions.get(i));
}
AtomicBoolean emptyWildcardExpansion = context.getOptions().allowNoIndices() ? null : new AtomicBoolean();
for (int i = firstWildcardIndex; i < expressions.size(); i++) {
String expression = expressions.get(i);
boolean isExclusion = i > firstWildcardIndex && expression.charAt(0) == '-';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a little lazy, could special case the first wildcard but it's even more code and not a measurable speedup given how complicated the loop body is, so I went for this. Either way, inlining the logic is about 3x faster than the existing logic from a quick experiment with the many-shards benchmark track.

if (i == firstWildcardIndex || isWildcard(expression)) {
Stream<IndexAbstraction> matchingResources = matchResourcesToWildcard(
context,
isExclusion ? expression.substring(1) : expression
);
Stream<String> matchingOpenClosedNames = expandToOpenClosed(context, matchingResources);
AtomicBoolean emptyWildcardExpansion = new AtomicBoolean(false);
if (context.getOptions().allowNoIndices() == false) {
if (emptyWildcardExpansion != null) {
emptyWildcardExpansion.set(true);
matchingOpenClosedNames = matchingOpenClosedNames.peek(x -> emptyWildcardExpansion.set(false));
}
if (expression.isExclusion()) {
matchingOpenClosedNames.forEachOrdered(result::remove);
if (isExclusion) {
matchingOpenClosedNames.forEach(result::remove);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it matters much, but no need to do these removals/additions in order.

} else {
matchingOpenClosedNames.forEachOrdered(result::add);
matchingOpenClosedNames.forEach(result::add);
}
if (emptyWildcardExpansion.get()) {
throw notFoundException(expression.get());
if (emptyWildcardExpansion != null && emptyWildcardExpansion.get()) {
throw notFoundException(expression);
}
} else {
if (expression.isExclusion()) {
result.remove(expression.get());
if (isExclusion) {
result.remove(expression.substring(1));
} else {
result.add(expression.get());
result.add(expression);
}
}
}
Expand Down Expand Up @@ -1507,27 +1526,35 @@ private DateMathExpressionResolver() {
// utility class
}

/**
* Resolves date math expressions. If this is a noop the given {@code expressions} list is returned without copying.
* As a result callers of this method should not mutate the returned list. Mutating it may come with unexpected side effects.
*/
public static List<String> resolve(Context context, List<String> expressions) {
List<String> result = new ArrayList<>(expressions.size());
for (ExpressionList.Expression expression : new ExpressionList(context, expressions)) {
result.add(resolveExpression(expression, context::getStartTime));
boolean wildcardSeen = false;
final boolean expandWildcards = context.getOptions().expandWildcardExpressions();
String[] result = null;
for (int i = 0, n = expressions.size(); i < n; i++) {
String expression = expressions.get(i);
// accepts date-math exclusions that are of the form "-<...{}>",f i.e. the "-" is outside the "<>" date-math template
boolean isExclusion = wildcardSeen && expression.startsWith("-");
wildcardSeen = wildcardSeen || (expandWildcards && isWildcard(expression));
String toResolve = isExclusion ? expression.substring(1) : expression;
String resolved = resolveExpression(toResolve, context::getStartTime);
if (toResolve != resolved) {
if (result == null) {
result = expressions.toArray(Strings.EMPTY_ARRAY);
}
result[i] = isExclusion ? "-" + resolved : resolved;
}
}
return result;
return result == null ? expressions : Arrays.asList(result);
}

static String resolveExpression(String expression) {
return resolveExpression(expression, System::currentTimeMillis);
}

static String resolveExpression(ExpressionList.Expression expression, LongSupplier getTime) {
if (expression.isExclusion()) {
// accepts date-math exclusions that are of the form "-<...{}>", i.e. the "-" is outside the "<>" date-math template
return "-" + resolveExpression(expression.get(), getTime);
} else {
return resolveExpression(expression.get(), getTime);
}
}

static String resolveExpression(String expression, LongSupplier getTime) {
if (expression.startsWith(EXPRESSION_LEFT_BOUND) == false || expression.endsWith(EXPRESSION_RIGHT_BOUND) == false) {
return expression;
Expand Down Expand Up @@ -1689,14 +1716,35 @@ private ExplicitResourceNameFilter() {
*/
public static List<String> filterUnavailable(Context context, List<String> expressions) {
ensureRemoteIndicesRequireIgnoreUnavailable(context.getOptions(), expressions);
List<String> result = new ArrayList<>(expressions.size());
for (ExpressionList.Expression expression : new ExpressionList(context, expressions)) {
validateAliasOrIndex(expression);
if (expression.isWildcard() || expression.isExclusion() || ensureAliasOrIndexExists(context, expression.get())) {
result.add(expression.expression());
final boolean expandWildcards = context.getOptions().expandWildcardExpressions();
boolean wildcardSeen = false;
List<String> result = null;
for (int i = 0; i < expressions.size(); i++) {
String expression = expressions.get(i);
if (Strings.isEmpty(expression)) {
throw notFoundException(expression);
}
// Expressions can not start with an underscore. This is reserved for APIs. If the check gets here, the API
// does not exist and the path is interpreted as an expression. If the expression begins with an underscore,
// throw a specific error that is different from the [[IndexNotFoundException]], which is typically thrown
// if the expression can't be found.
if (expression.charAt(0) == '_') {
throw new InvalidIndexNameException(expression, "must not start with '_'.");
}
final boolean isWildcard = expandWildcards && isWildcard(expression);
if (isWildcard || (wildcardSeen && expression.charAt(0) == '-') || ensureAliasOrIndexExists(context, expression)) {
if (result != null) {
result.add(expression);
}
} else {
if (result == null) {
result = new ArrayList<>(expressions.size() - 1);
result.addAll(expressions.subList(0, i));
}
}
wildcardSeen |= isWildcard;
}
return result;
return result == null ? expressions : result;
}

/**
Expand Down Expand Up @@ -1736,19 +1784,6 @@ private static boolean ensureAliasOrIndexExists(Context context, String name) {
return true;
}

private static void validateAliasOrIndex(ExpressionList.Expression expression) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No point in having a single use method like that not inside the loop body, it's just harder on the compiler a well as the reader of the code.

if (Strings.isEmpty(expression.expression())) {
throw notFoundException(expression.expression());
}
// Expressions can not start with an underscore. This is reserved for APIs. If the check gets here, the API
// does not exist and the path is interpreted as an expression. If the expression begins with an underscore,
// throw a specific error that is different from the [[IndexNotFoundException]], which is typically thrown
// if the expression can't be found.
if (expression.expression().charAt(0) == '_') {
throw new InvalidIndexNameException(expression.expression(), "must not start with '_'.");
}
}

private static void ensureRemoteIndicesRequireIgnoreUnavailable(IndicesOptions options, List<String> indexExpressions) {
if (options.ignoreUnavailable()) {
return;
Expand All @@ -1773,57 +1808,6 @@ private static void failOnRemoteIndicesNotIgnoringUnavailable(List<String> index
}
}

/**
* Used to iterate expression lists and work out which expression item is a wildcard or an exclusion.
*/
public static final class ExpressionList implements Iterable<ExpressionList.Expression> {
private final List<Expression> expressionsList;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no need for this intermediary list, inlining the logic and doing the checks on the fly is at least twice as fast as this approach and O(1) instead of O(n) in heap use.

private final boolean hasWildcard;

public record Expression(String expression, boolean isWildcard, boolean isExclusion) {
public String get() {
if (isExclusion()) {
// drop the leading "-" if exclusion because it is easier for callers to handle it like this
return expression().substring(1);
} else {
return expression();
}
}
}

/**
* Creates the expression iterable that can be used to easily check which expression item is a wildcard or an exclusion (or both).
* The {@param context} is used to check if wildcards ought to be considered or not.
*/
public ExpressionList(Context context, List<String> expressionStrings) {
List<Expression> expressionsList = new ArrayList<>(expressionStrings.size());
boolean wildcardSeen = false;
for (String expressionString : expressionStrings) {
boolean isExclusion = expressionString.startsWith("-") && wildcardSeen;
if (context.getOptions().expandWildcardExpressions() && isWildcard(expressionString)) {
wildcardSeen = true;
expressionsList.add(new Expression(expressionString, true, isExclusion));
} else {
expressionsList.add(new Expression(expressionString, false, isExclusion));
}
}
this.expressionsList = expressionsList;
this.hasWildcard = wildcardSeen;
}

/**
* Returns {@code true} if the expression contains any wildcard and the options allow wildcard expansion
*/
public boolean hasWildcard() {
return this.hasWildcard;
}

@Override
public Iterator<ExpressionList.Expression> iterator() {
return expressionsList.iterator();
}
}

/**
* This is a context for the DateMathExpressionResolver which does not require {@code IndicesOptions} or {@code ClusterState}
* since it uses only the start time to resolve expressions.
Expand Down
Loading