Speed up InternalEngine#resolveDocVersion(...) method #122374
Conversation
Use reference manager to get index reader instead of acquire a searcher. The latter involved creating an index searcher, which is not used and expensive as part og resolveDocVersion() method.
martijnvg
added
:Distributed Indexing/Engine
|
Hi @martijnvg, I've created a changelog YAML for you. |
elasticsearchmachine
added
Team:StorageEngine
Team:Distributed Indexing
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
| final VersionsAndSeqNoResolver.DocIdAndVersion docIdAndVersion; | ||
| try (Searcher searcher = acquireSearcher("load_version", SearcherScope.INTERNAL)) { | ||
| var indexReader = referenceManager.acquire(); |
There was a problem hiding this comment.
I think we need to hold a reference to the store before acquiring the referenceManager.
There was a problem hiding this comment.
Strange that no test then failed.
There was a problem hiding this comment.
I think it's very unlikely and might even be impossible to run into a closed store here?
There was a problem hiding this comment.
Yes, resolveDocVersion is executed while holding a ref on the engine (preventing the engine to be closed and in turn the store to be closed too).
martijnvg
force-pushed
the
InternalEngine_resolveDocVersion_method
branch
from
c0a0199 to
8072907
Compare
|
I swapped the enhancement label with non-issue label. After further analysis the reason why constructing an This is still the right change, the |
| /* Acquire order here is store -> manager since we need | ||
| * to make sure that the store is not closed before | ||
| * the searcher is acquired. */ | ||
| if (store.tryIncRef() == false) { |
There was a problem hiding this comment.
Ah see @tlrx, we don't need to ever acquire a store because we are guaranteed to have an engine reference here?
There was a problem hiding this comment.
I see, so we can just change that into an assertion?
There was a problem hiding this comment.
I think so yes, assertion should be enough :)
| } | ||
| } | ||
|
|
||
| abstract static class DirectoryReaderSupplier implements Releasable { |
There was a problem hiding this comment.
This is only extended once, we can make this one class for the reader supplier can't we? Or better yet, maybe just inline the logic to save some cycles? :)
martijnvg
added
v9.0.1
auto-backport
Use reference manager to get index reader instead of acquiring a searcher. The latter involves creating an index searcher, which is not used and expensive as part of the `resolveDocVersion(...)` method, because this method is invoked for each document that gets indexed.
Use reference manager to get index reader instead of acquiring a searcher. The latter involves creating an index searcher, which is not used and expensive as part of the `resolveDocVersion(...)` method, because this method is invoked for each document that gets indexed.
Checking whether we need to refresh does not require a searcher so we can simplify this to just work based on the reader and avoid lots of contention etc. for setting up the searcher. relates #122374
Checking whether we need to refresh does not require a searcher so we can simplify this to just work based on the reader and avoid lots of contention etc. for setting up the searcher. relates elastic#122374
Use reference manager to get index reader instead of acquire a searcher. The latter involves creating an index searcher, which is not used and expensive as part of the
resolveDocVersion(...)method. This method is invoked for each document that gets indexed.