[Improvement]record split across writer buffer to save memory

[summaryzb]

What changes were proposed in this pull request?

Record added to WriterBuffer will split across byte array buffer, all the byte array will be fully used

Why are the changes needed?

Previously for example, the length of every record is 2k, every time we add a record, we create a buffer with 3k length by default and wrap the previous buffer as WrappedBuffer which resulting in wasting 1k memory in every WrappedBuffer.
Apply this PR will save memory

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass unit test

[summaryzb]

@colinmjj @jerqi PTAL

[codecov-commenter]

Codecov Report

Merging #157 (e65178f) into master (f49b566) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #157      +/-   ##
============================================
+ Coverage     57.38%   57.43%   +0.04%     
+ Complexity     1208     1205       -3     
============================================
  Files           150      150              
  Lines          8209     8196      -13     
  Branches        775      771       -4     
============================================
- Hits           4711     4707       -4     
+ Misses         3255     3249       -6     
+ Partials        243      240       -3     

Impacted Files	Coverage Δ
...pache/spark/shuffle/writer/WriteBufferManager.java	82.67% <100.00%> (-0.79%)	⬇️
.../org/apache/spark/shuffle/writer/WriterBuffer.java	100.00% <100.00%> (+6.52%)	⬆️
...apache/uniffle/coordinator/ApplicationManager.java	83.70% <0.00%> (+2.88%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[colinmjj]

How about:

// comments
if (require > 0) {
  // comments
  if (buffer != null) {
     int hasCopied = xxx;
     // commnets
     if (hasCopied > 0) {
     }
  }
}

[summaryzb]

Follow this suggestion

[colinmjj]

@summaryzb For this optimization, I think there has once more System.arraycopy to add record, and it will impact performance a lot with previous test.
For this PR, I think it is target to make size of block larger than before. To reduce memory pressure in client side, how about change the parameter for flush strategy?

[summaryzb]

@summaryzb For this optimization, I think there has once more System.arraycopy to add record, and it will impact performance a lot with previous test. For this PR, I think it is target to make size of block larger than before. To reduce memory pressure in client side, how about change the parameter for flush strategy?

Yeah,it indeed has once more System.arraycopy when record splits across writer buffer, however it result in reducing the num of System.arraycopy when transfer all the buffer into one byte array. Additionally, we should consider the total System.arraycopy for a certain total number of bytes to be written, rather than for a certain buffer.

[colinmjj]

@summaryzb For this optimization, I think there has once more System.arraycopy to add record, and it will impact performance a lot with previous test. For this PR, I think it is target to make size of block larger than before. To reduce memory pressure in client side, how about change the parameter for flush strategy?

Yeah,it indeed has once more System.arraycopy when record splits across writer buffer, however it result in reducing the num of System.arraycopy when transfer all the buffer into one byte array. Additionally, we should consider the total System.arraycopy for a certain total number of bytes to be written, rather than for a certain buffer.

For the situation with buffer = 3k, length of record = 2.8k, after insert 1000 records:
with current implementation, calls of System.arraycopy is about 2000(include insert & merge)
with PR, calls of System.arraycopy is about 2800
I agree that memory waste happen in your example, but I prefer to avoid possible performance regression with more resources.

[summaryzb]

but I prefer to avoid possible performance regression with more resources.
Well, how about adding a config to make this pr as an optional strategy, while current implementation is a default choice

[colinmjj]

In spark client, all memory are requested from executor, so there shouldn't have critical problem, eg, memory leak, oom, etc.
Can you show the case how this PR improve the application, for example, X% performance improvement with this PR.

[summaryzb]

no obvious benefit can be gather from this pr, close it