[ISSUE-455] Lazily create uncompressedData #457
Conversation
Codecov Report
@@ Coverage Diff @@
## master #457 +/- ##
============================================
+ Coverage 58.67% 61.74% +3.06%
- Complexity 1654 1655 +1
============================================
Files 199 193 -6
Lines 11217 10053 -1164
Branches 997 997
============================================
- Hits 6582 6207 -375
+ Misses 4243 3514 -729
+ Partials 392 332 -60 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java
Show resolved
Hide resolved
| // todo: support off-heap bytebuffer | ||
| uncompressedData = ByteBuffer.allocate( | ||
| (int) rssConf.getSizeAsBytes( | ||
| RssClientConfig.RSS_WRITER_BUFFER_SIZE, |
There was a problem hiding this comment.
this should be RSS_READER_BUFFER_SIZE?
And for this if block, the bufferSize should be max(readBufferSize, uncompressedLen)?
Could you add a test case for this scenario?
There was a problem hiding this comment.
I think the purpose of using RSS_WRITER_BUFFER_SIZE may be to avoid allocate memory for multiple times.Let's remove it.
There was a problem hiding this comment.
Yes. The initial uncompressedData size is hard to set. I prefer to remove it.
| @@ -69,13 +67,6 @@ public RssShuffleDataIterator( | |||
| this.shuffleReadClient = shuffleReadClient; | |||
| this.shuffleReadMetrics = shuffleReadMetrics; | |||
| this.codec = Codec.newInstance(rssConf); | |||
| // todo: support off-heap bytebuffer | |||
There was a problem hiding this comment.
We would better not remove this todo comment.
|
Merged. Thanks @jerqi @xianjingfeng @advancedxy |
What changes were proposed in this pull request?
Lazily create
uncompressedData.Why are the changes needed?
Save memory. #455
Does this PR introduce any user-facing change?
No
How was this patch tested?
The existing UTs is enough