Reduce memory usage on Azure repository implementation #66489
Merged
Conversation
This file contains 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
This commit moves the upload logic to the repository itself instead of delegating into the SDK. Multi-block uploads are done sequentially instead of in parallel that allows to bound the outstanding memory. Additionally the number of i/o threads have been reduced to 1, to reduce the memory overhead.
fcofdez
added
>enhancement
:Distributed Coordination/Snapshot/Restore
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
fcofdez
commented
Dec 17, 2020
| @@ -150,7 +150,6 @@ private static ByteBufAllocator createByteBufAllocator() { | |||
| int tinyCacheSize = PooledByteBufAllocator.defaultTinyCacheSize(); | |||
| int smallCacheSize = PooledByteBufAllocator.defaultSmallCacheSize(); | |||
| int normalCacheSize = PooledByteBufAllocator.defaultNormalCacheSize(); | |||
| boolean useCacheForAllThreads = PooledByteBufAllocator.defaultUseCacheForAllThreads(); | |||
|
|
|||
| return new PooledByteBufAllocator(false, | |||
| nHeapArena, | |||
There was a problem hiding this comment.
I think that we can reduce those too, wdyt @original-brownbear?
There was a problem hiding this comment.
Yea, can't we just do 1 maybe when we only use one thread now anyway?
There was a problem hiding this comment.
Thanks Francisco. I only have one question pretty much.
That said, I think the move to a single thread and reducing the heap arenas are straight forward in any case. Lets see what we can do about the upload flux. I guess if that turns out to be trickier we could pull the arena and thread count changes to a separate PR so we can reenable tests asap maybe?
| @@ -150,7 +150,6 @@ private static ByteBufAllocator createByteBufAllocator() { | |||
| int tinyCacheSize = PooledByteBufAllocator.defaultTinyCacheSize(); | |||
| int smallCacheSize = PooledByteBufAllocator.defaultSmallCacheSize(); | |||
| int normalCacheSize = PooledByteBufAllocator.defaultNormalCacheSize(); | |||
| boolean useCacheForAllThreads = PooledByteBufAllocator.defaultUseCacheForAllThreads(); | |||
|
|
|||
| return new PooledByteBufAllocator(false, | |||
| nHeapArena, | |||
There was a problem hiding this comment.
Yea, can't we just do 1 maybe when we only use one thread now anyway?
| int numOfBytesRead = 0; | ||
| int offset = 0; | ||
| int len = (int) count; | ||
| final byte[] buffer = new byte[len]; |
There was a problem hiding this comment.
This still scares me a little. If we only do 64k at a time here, can't we use the Netty memory allocator (or manage our own set of byte[] and recycle them on doOnComplete or do we still have no guarantees about flushing at that point?
There was a problem hiding this comment.
Sadly we don't have guarantees about flushing at that point
There was a problem hiding this comment.
I explored a different approach where I was passing an allocator there, and recycling at the end of the request, but in that case you end up holding that memory for the entire duration of the request.
original-brownbear
approved these changes
Dec 17, 2020
fcofdez
added a commit
to fcofdez/elasticsearch
that referenced
this pull request
Dec 17, 2020
This commit moves the upload logic to the repository itself instead of delegating into the SDK. Multi-block uploads are done sequentially instead of in parallel that allows to bound the outstanding memory. Additionally the number of i/o threads and heap arenas have been reduced to 1, to reduce the memory overhead. Closes elastic#66385 Backport of elastic#66489
fcofdez
added a commit
to fcofdez/elasticsearch
that referenced
this pull request
Dec 17, 2020
This commit moves the upload logic to the repository itself instead of delegating into the SDK. Multi-block uploads are done sequentially instead of in parallel that allows to bound the outstanding memory. Additionally the number of i/o threads and heap arenas have been reduced to 1, to reduce the memory overhead. Closes elastic#66385 Backport of elastic#66489
fcofdez
added a commit
that referenced
this pull request
Dec 17, 2020
This commit moves the upload logic to the repository itself instead of delegating into the SDK. Multi-block uploads are done sequentially instead of in parallel that allows to bound the outstanding memory. Additionally the number of i/o threads and heap arenas have been reduced to 1, to reduce the memory overhead. Closes #66385 Backport of #66489
fcofdez
added a commit
that referenced
this pull request
Dec 17, 2020
This commit moves the upload logic to the repository itself instead of delegating into the SDK. Multi-block uploads are done sequentially instead of in parallel that allows to bound the outstanding memory. Additionally the number of i/o threads and heap arenas have been reduced to 1, to reduce the memory overhead. Closes #66385 Backport of #66489
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Feb 12, 2021
Today we represent block IDs sent to Azure using the URL-safe base-64 encoding. This makes sense: these IDs appear in URLs. It turns out that Azure rejects this encoding for block IDs and instead demands that they are represented using the regular, URL-unsafe, base-64 encoding instead, then further wrapped in %-encoding to deal with the URL-unsafe characters that inevitably result. Relates elastic#66489
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Feb 15, 2021
Today we represent block IDs sent to Azure using the URL-safe base-64 encoding. This makes sense: these IDs appear in URLs. It turns out that Azure rejects this encoding for block IDs and instead demands that they are represented using the regular, URL-unsafe, base-64 encoding instead, then further wrapped in %-encoding to deal with the URL-unsafe characters that inevitably result. Relates elastic#66489 Backport of elastic#68957
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Feb 15, 2021
Today we represent block IDs sent to Azure using the URL-safe base-64 encoding. This makes sense: these IDs appear in URLs. It turns out that Azure rejects this encoding for block IDs and instead demands that they are represented using the regular, URL-unsafe, base-64 encoding instead, then further wrapped in %-encoding to deal with the URL-unsafe characters that inevitably result. Relates elastic#66489 Backport of elastic#68957
DaveCTurner
added a commit
that referenced
this pull request
Feb 15, 2021
Today we represent block IDs sent to Azure using the URL-safe base-64 encoding. This makes sense: these IDs appear in URLs. It turns out that Azure rejects this encoding for block IDs and instead demands that they are represented using the regular, URL-unsafe, base-64 encoding instead, then further wrapped in %-encoding to deal with the URL-unsafe characters that inevitably result. Relates #66489
DaveCTurner
added a commit
that referenced
this pull request
Feb 15, 2021
Today we represent block IDs sent to Azure using the URL-safe base-64 encoding. This makes sense: these IDs appear in URLs. It turns out that Azure rejects this encoding for block IDs and instead demands that they are represented using the regular, URL-unsafe, base-64 encoding instead, then further wrapped in %-encoding to deal with the URL-unsafe characters that inevitably result. Relates #66489 Backport of #68957
DaveCTurner
added a commit
that referenced
this pull request
Feb 15, 2021
Today we represent block IDs sent to Azure using the URL-safe base-64 encoding. This makes sense: these IDs appear in URLs. It turns out that Azure rejects this encoding for block IDs and instead demands that they are represented using the regular, URL-unsafe, base-64 encoding instead, then further wrapped in %-encoding to deal with the URL-unsafe characters that inevitably result. Relates #66489 Backport of #68957
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 commit moves the upload logic to the repository itself
instead of delegating into the SDK.
Multi-block uploads are done sequentially instead of in parallel
that allows to bound the outstanding memory.
Additionally the number of i/o threads have been reduced to 1,
to reduce the memory overhead.
Closes #66385