[#1472][part-4] feature(server): Introduce metrics for Netty's pinnedDirectMemory and usedDirectMemory #1524
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
…DirectMemory and usedDirectMemory
|
@jerqi PTAL |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1524 +/- ##
============================================
+ Coverage 54.20% 55.19% +0.99%
Complexity 2808 2808
============================================
Files 430 410 -20
Lines 24410 22058 -2352
Branches 2082 2082
============================================
- Hits 13231 12175 -1056
+ Misses 10349 9124 -1225
+ Partials 830 759 -71 ☔ View full report in Codecov by Sentry. |
jerqi
reviewed
Feb 13, 2024
| @@ -79,6 +79,8 @@ public class ShuffleServerMetrics { | |||
| private static final String USED_BUFFER_SIZE = "used_buffer_size"; | |||
| private static final String READ_USED_BUFFER_SIZE = "read_used_buffer_size"; | |||
| private static final String USED_DIRECT_MEMORY_SIZE = "used_direct_memory_size"; | |||
| private static final String ALLOCATED_DIRECT_MEMORY_SIZE = "allocated_direct_memory_size"; | |||
| private static final String PINNED_DIRECT_MEMORY_SIZE = "pinned_direct_memory_size"; | |||
There was a problem hiding this comment.
What's the meaning of pinned?
There was a problem hiding this comment.
Refer to netty/netty#11667.
To be brief, it means the direct memory actually used by PooledByteBufAllocator, cannot be used anymore. If we use the old metric usedDirectMemory, we cannot tell whether the direct memory is cached by PooledByteBufAllocator or if it's truly exhausted.
jerqi
approved these changes
Feb 13, 2024
zuston
pushed a commit
that referenced
this pull request
Feb 23, 2024
…mory issue causing OOM (#1534) ### What changes were proposed in this pull request? When we use `UnpooledByteBufAllocator` to allocate off-heap `ByteBuf`, Netty directly requests off-heap memory from the operating system instead of allocating it according to `pageSize` and `chunkSize`. This way, we can obtain the exact `ByteBuf` size during the pre-allocation of memory, avoiding distortion of metrics such as `usedMemory`. Moreover, we have restored the code submission of the PR [#1521](#1521). We ensure that there is sufficient direct memory for the Netty server during decoding `sendShuffleDataRequest` by taking into account the `encodedLength` of `ByteBuf` in advance during the pre-allocation of memory, thus avoiding OOM during decoding `sendShuffleDataRequest`. Since we are not using `PooledByteBufAllocator`, the PR [#1524](#1524) is no longer needed. ### Why are the changes needed? A sub PR for: #1519 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing UTs.
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.
What changes were proposed in this pull request?
We need to know the exact direct memory usage of
PooledByteBufAllocatorinNetty.So we should introduce metrics for Netty's
pinnedDirectMemoryandusedDirectMemory.Why are the changes needed?
A sub PR for: #1519
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing UTs.