(celeborn) branch branch-0.5 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 branch-0.5
in repository https://gitbox.apache.org/repos/asf/celeborn.git


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

commit 136dde5158e24331b7a10374e321bc80e5dca8f3
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>
    (cherry picked from commit 193dc6cf8b5ee3d48f27313bd2a96856d8ea8d81)
    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 4c5e6739b..84e58b15b 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
@@ -330,8 +330,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);