Stop Allocating Document Source to Unpooled Buffers when Indexing #67502
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WIP, this was a holiday project and still needs some cleanup here and there and should be broken up into pieces since it introduces tricky new test infrastructure. But overall this commit works. It makes it so all the source bytes in bulk requests are shared throughout the lifecycle of the bulk request instead of copying them to unpooled buffers when deserializing indexing requests. The essentially cuts the peak memory use for bulk requests in almost half (though by retaining buffers for longer that effect is reduced a little compared to GC's ability to more fine-grained collected byte arrays). Still overall this reduces GC times by 2/3 when running the rally PMC indexing track, reduces the risk of humungous allocations with G1GC and makes memory use less spiky so on balance I think this is the direction to go.
original-brownbear
added
>non-issue
WIP
:Distributed Indexing/CRUD
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Jan 19, 2021
This commit adds leak tracking infrastructure that enables assertions about the state of objects at GC time (simplified version of what Netty uses to track `ByteBuf` instances). This commit uses the infrastructure to improve the quality of leak checks for page recycling in the mock nio transport (the logic in `org.elasticsearch.common.util.MockPageCacheRecycler#ensureAllPagesAreReleased` does not run for all tests and tracks too little information to allow for debugging what caused a specific leak in most cases due to the lack of an equivalent of the added `#touch` logic). This is elastic#67502
Contributor
|
@elasticmachine update branch |
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Feb 25, 2021
Added this assertion to have an easier time debugging work on elastic#67502 and found that we were accessing `refcount == 0` bytes in the `SSLOutboundBuffer` so I fixed that buffer to not keep references to released pages.
original-brownbear
added a commit
that referenced
this pull request
Feb 25, 2021
Added this assertion to have an easier time debugging work on #67502 and found that we were accessing `refcount == 0` bytes in the `SSLOutboundBuffer` so I fixed that buffer to not keep references to released pages.
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Feb 25, 2021
Added this assertion to have an easier time debugging work on elastic#67502 and found that we were accessing `refcount == 0` bytes in the `SSLOutboundBuffer` so I fixed that buffer to not keep references to released pages.
original-brownbear
added a commit
that referenced
this pull request
Feb 25, 2021
elasticmachine
added
the
Team:Distributed (Obsolete)
Collaborator
|
Pinging @elastic/es-distributed (Team:Distributed) |
| TimeValue next = backoff.next(); | ||
| logger.trace("Retry of bulk request scheduled in {} ms.", next.millis()); | ||
| retryCancellable = scheduler.schedule(() -> this.execute(bulkRequestForRetry), next, ThreadPool.Names.SAME); | ||
| bulkRequestForRetry.incRef(); |
Contributor
Author
There was a problem hiding this comment.
I initially thought this was inefficient relative to what we currently do but:
Retrying, will cause the whole request to be retained instead of just those requests that actually get retried. But, I think in the real world this is less of an issue with where we currently are.
If the retry happens on a coordinating node, we already have this problem (since we use shared bytes that are tracked as one thing for the whole bulk request). If the retry happens for a replication request, then the replication request is the same unit of work as the primary bulk request received so that seems fine as well. There's certainly room here for more selectively retaining bytes though but I don't think this PR makes us worse off here.
Contributor
Author
|
Jenkins run elasticsearch-ci/2 (unrelated ml) |
Contributor
Author
|
Jenkins run elasticsearch-ci/1 |
Collaborator
|
Pinging @elastic/es-distributed-obsolete (Team:Distributed (Obsolete)) |
elasticsearchmachine
added
the
Team:Distributed Coordination
Collaborator
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
18 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes the handling of bulk shard requests stop allocating a new
byte[]for every document source. Doing so effectively doubles the peak memory use for any bulk request not handled on a coordinating node (there the bytes are already pooled via the REST layer) needlessly. With larger documents allocating the source to unpooledbyte[]could also lead to humongous allocations for the document source.Unlike the coordinator node pooling of bytes, pooling these requests per shard should behave a lot nicer in theory since the requests (
BulkShardRequest) get processed "in one piece" anyway and need to be retained fully for retries as well as far as I can see.In some quick and dirty benchmarking I could see a significant drop in GC for the PMC benchmark run on a 3-node cluster with this and a massive saving in the number of
byte[]that are allocated per unit of time.