-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Some obvious speedups to parsing alias filters #127240
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.
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
Some obvious speedups to parsing alias filters #127240
Conversation
It's in the title, saving some allocations and weird indirection in obvious spots.
Pinging @elastic/es-data-management (Team:Data Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added two questions purely for my understanding, but LGTM. Thanks Armin!
Predicate<String> isMatchAll = (((Predicate<String>) Metadata.ALL::equals)).or(Regex::isMatchAllPattern); | ||
if (expressions == null | ||
|| expressions.length == 0 | ||
|| expressions.length == 1 | ||
&& (SelectorResolver.selectorsValidatedAndMatchesPredicate(expressions[0], context, isMatchAll))) { | ||
&& (SelectorResolver.selectorsValidatedAndMatchesPredicate( | ||
expressions[0], | ||
context, | ||
s -> Metadata.ALL.equals(s) || Regex.isMatchAllPattern(s) | ||
))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, hat's the value of inlining this variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metadata.all::equals and the .or
call both actually allocate an object so this removes allocation outright (as well as some overhead for the method handles).
Also, the .or
doesn't compile anything really, it just chains two predicates generically, unless that inlines we're just burning extra cycles for method calls vs a non-capturing lambda :)
List<QueryBuilder> filters = Arrays.stream(aliases).map(key -> { | ||
var f = dsAliases.get(key).getFilter(dataStreamName); | ||
if (f == null) { | ||
return null; | ||
} | ||
try { | ||
return parseFilter(f.compressedReference()); | ||
} catch (IOException e) { | ||
throw new UncheckedIOException(e); | ||
} | ||
}).filter(Objects::nonNull).toList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the value of reordering these stream operations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We save one lookup on the aliases map by not doing an extra one just to null check :)
Thanks Niels! |
It's in the title, saving some allocations and weird indirection in obvious spots.