Use coarse grained memory reporting to reduce congestion #22841
Conversation
There was a problem hiding this comment.
LGTM, except for some minor comments. cc: @findepi - this avoids under-reporting the amount of memory used which can be dangerous at high concurrency.
|
|
||
| // gets set to the next coarse unit | ||
| // k*syncThreshold + x in [1, syncThreshold] leads to setting the context to (k+1) * syncThreshold | ||
| Random random = new Random(); |
There was a problem hiding this comment.
Using Random and asserting that k * syncThreshold + x == (k + 1) * syncThreshold is probably overkill here since the implementation itself is fairly straightforward math. Let's go with some hard coded example values and then maybe add a loop to assert that for all values in the range of [1, syncThreshold] the result is syncThreshold
| { | ||
| this(delegate, DEFAULT_SYNC_THRESHOLD); | ||
| } | ||
|
|
||
| public ThresholdLocalMemoryContext(LocalMemoryContext delegate, long syncThreshold) | ||
| public CoarseGrainLocalMemoryContext(LocalMemoryContext delegate, long syncThreshold) |
There was a problem hiding this comment.
Nit: maybe rename syncThreshold to granularity. If renamed, also update the instance field name and DEFAULT_SYNC_THRESHOLD constant names too.
| * In this paradigm, the most small incremental increases at the byte granularity will not actually result | ||
| * in a different coarse granularity value, so no reporting into memory tracking is necessary | ||
| */ | ||
| public class CoarseGrainLocalMemoryContext |
There was a problem hiding this comment.
All LocalMemoryContext implementations are thread safe. I think this one should be too cc @losipiuk
|
thanks for the review @pettyjamesm @sopel39 -- addressed the comments. |
| public CoarseGrainLocalMemoryContext(LocalMemoryContext delegate, long granularity) | ||
| { | ||
| this.delegate = requireNonNull(delegate, "delegate is null"); | ||
| checkArgument(granularity > 0, "syncThreshold must be greater than 0"); |
There was a problem hiding this comment.
"granularity must be greater than 0"
This reduces congestion in the memory tracking system by coarsening the granularity of memory tracking. In this paradigm, the most small incremental increases at the byte granularity will not actually result in a different coarse granularity value, so no reporting into memory tracking is necessary. Co-authored-by: James Petty <pettja@amazon.com>
ShubhamChaurasia
force-pushed
the
coarse-grain-memory-context
branch
from
b40db86
to
526cfcb
Compare
| private final LocalMemoryContext delegate; | ||
| private final long granularity; | ||
| private final long mask; | ||
| private long currentBytes; |
| @@ -1,93 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
nit: this should be two commits:
- rename
- changes on top
this way history is preserved
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| /** | ||
| * This class prevents contention in the memory tracking system by coarsening the granularity of memory tracking. |
There was a problem hiding this comment.
nit: Possible typo (description says "congestion," here we say "contention").
| assertCoarseGrainContextValues(coarseGrainContext, delegate, i, granularity); | ||
| } | ||
|
|
||
| // threshold not a power of 2 |
|
Does this need release notes? cc @pettyjamesm |
|
Sorry about that @colebow, I should have verified the release notes section. No notes should be necessary on this one. |
Description
This reduces congestion in the memory tracking system by coarsening the granularity of memory tracking. In this paradigm, the most small incremental increases at the byte granularity will not actually result in a different coarse granularity value, so no reporting into memory tracking is necessary.
This has already been addressed previously in #22668, however it under reports memory to the tracking system which can be a problem in high concurrency environment.
In this approach, we prefer to over-report than under report within the small rounding margin. Also, this eliminates the need of an explicit
sync()API.Thanks to @pettyjamesm for the idea and the guidance for this.
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: