Remove SearchOperationListenerExecutor abstraction #124298
Conversation
This isn't even saving any lines of code and is a measurable source of both cache and branch-prediction misses on the hot and critical transport-thread path. => inline it
This isn't even saving any lines of code and is a measurable source of both cache and branch-prediction misses on the hot and critical transport-thread path. => inline it
original-brownbear
added
>non-issue
:Search Foundations/Search
elasticsearchmachine
added
the
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
| @@ -760,13 +760,21 @@ private SearchPhaseResult executeQueryPhase(ShardSearchRequest request, Cancella | |||
| ) { | |||
| tracer.startTrace("executeQueryPhase", Map.of()); | |||
| final long afterQueryTime; | |||
| try (SearchOperationListenerExecutor executor = new SearchOperationListenerExecutor(context)) { | |||
| final long beforeQueryTime = System.nanoTime(); | |||
There was a problem hiding this comment.
The strategy pattern can be implemented here to avoid the extra branching and the boilerplate code in the SearchService class.
There was a problem hiding this comment.
Right, but if you think about it, that will probably not save any lines of code and will most likely be heavier on the CPU still than just inlining the logic here (at best that's what the compiler will get us to with escape analysis and inlining).
-> I think I'd vote for just inlining here even if it looks less nice simple because we're already having trouble with the i-cache and as a result a predictable improvement trumps deduplication? (it's also not like this logic will ever see its use expand, so any abstractions we add have like 3 uses and will in all likelihood never see any additional uses either)
There was a problem hiding this comment.
I don't have a strong opinion here. This PR looks more straightforward to read and follow than it did before. I just proposed a more object-oriented solution to make it more eye-friendly.
| searchContext.addQueryResult(); | ||
| QueryPhase.execute(searchContext); | ||
| final long afterQueryTime = executor.success(); | ||
| var opsListener = readerContext.indexShard().getSearchOperationListener(); |
There was a problem hiding this comment.
To align with the rest of the cases, you can add it to the try-catch before the onPreQueryPhase call.
…utor' into drop-SearchOperationListenerExecutor
original-brownbear
added
the
auto-backport
|
Thanks Dimitris! |
💚 Backport successful
|
This isn't even saving any lines of code and is a measurable source of both cache and branch-prediction misses on the hot and critical transport-thread path. => inline it
This isn't even saving any lines of code and is a measurable source of both cache and branch-prediction misses on the hot and critical transport-thread path. => inline it
This isn't even saving any lines of code and is a measurable source of both cache and branch-prediction misses on the hot and critical transport-thread path. => inline it
This isn't even saving any lines of code and is a measurable source of both cache and branch-prediction misses on the hot and critical transport-thread path.
=> inline it and save some branches and instruction cache waste