(celeborn) branch main updated: [CELEBORN-1929] Avoid unnecessary buffer loss to get better buffer reusability

[feiwang]

This is an automated email from the ASF dual-hosted git repository.

feiwang pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/celeborn.git


The following commit(s) were added to refs/heads/main by this push:
     new 193dc6cf8 [CELEBORN-1929] Avoid unnecessary buffer loss to get better 
buffer reusability
193dc6cf8 is described below

commit 193dc6cf8b5ee3d48f27313bd2a96856d8ea8d81
Author: Saurabh Dubey <saurabhd...@uber.com>
AuthorDate: Sun Mar 30 23:53:48 2025 -0700

    [CELEBORN-1929] Avoid unnecessary buffer loss to get better buffer 
reusability
    
    ### What changes were proposed in this pull request?
    There is an unnecessary explicit `buffer[i] = null` that would cause 
certain set of buffers to not be reused from the pool, leading to higher memory 
usage and gc pressure
    
    ### Why are the changes needed?
    https://github.com/apache/celeborn/pull/131 introduced a feature to reuse 
buffers across mapper tasks. However it seems like when closing the writer, we 
explicitly set certain (i.e. buffers which are non empty and need to be flushed 
when closing) buffer to null, leading to these buffers not being returned to 
the pool.
    
    As a result, such buffers are permanently lost (GC'ed), and we'd end up 
creating more buffers that really necessary 
[here](https://github.com/apache/celeborn/blob/main/client-spark/spark-2/src/main/java/org/apache/spark/shuffle/celeborn/HashBasedShuffleWriter.java#L270),
 which contributes to the overall total memory usage.
    
    I checked the flow and the buffer being merged, is compressed 
[here](https://github.com/apache/celeborn/blob/main/client/src/main/java/org/apache/celeborn/client/ShuffleClientImpl.java#L1019)
 and then copied into a new buffer 
[here](https://github.com/apache/celeborn/blob/main/client/src/main/java/org/apache/celeborn/client/ShuffleClientImpl.java#L1031),
 and all of these are sync operations i.e. the overwrite to the buffer by a 
newly started writer task, should not cause any data corr [...]
    
    ### Does this PR introduce _any_ user-facing change?
    No
    
    ### How was this patch tested?
    TODO (add UTs), but manually tested once
    
    Closes #3173 from saurabhd336/bufferReuse.
    
    Authored-by: Saurabh Dubey <saurabhd...@uber.com>
    Signed-off-by: Wang, Fei <fwan...@ebay.com>
---
 .../java/org/apache/spark/shuffle/celeborn/HashBasedShuffleWriter.java  | 2 --
 1 file changed, 2 deletions(-)

diff --git 
a/client-spark/spark-3/src/main/java/org/apache/spark/shuffle/celeborn/HashBasedShuffleWriter.java
 
b/client-spark/spark-3/src/main/java/org/apache/spark/shuffle/celeborn/HashBasedShuffleWriter.java
index 0202f2f5f..c423a97ce 100644
--- 
a/client-spark/spark-3/src/main/java/org/apache/spark/shuffle/celeborn/HashBasedShuffleWriter.java
+++ 
b/client-spark/spark-3/src/main/java/org/apache/spark/shuffle/celeborn/HashBasedShuffleWriter.java
@@ -333,8 +333,6 @@ public class HashBasedShuffleWriter<K, V, C> extends 
ShuffleWriter<K, V> {
       final int size = sendOffsets[i];
       if (size > 0) {
         mergeData(i, sendBuffers[i], 0, size);
-        // free buffer
-        sendBuffers[i] = null;
       }
     }
     sendBufferPool.returnBuffer(sendBuffers);