Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MINOR] refactor: Reduce the usage of memory in the ShuffleWriter #877

Merged
merged 2 commits into from
May 12, 2023

Conversation

jerqi
Copy link
Contributor

@jerqi jerqi commented May 12, 2023

What changes were proposed in this pull request?

We should immediately remove the WriteBuffer after we use the method createShuffleBlock to create ShuffeBlock. Otherwise we will have the compressed data and uncompressed data at the same time in the memory. JVM's GC can't collect the memory of uncompressed data, because the data have the reference in the map buffers.

Why are the changes needed?

Reduce the usage of memory.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

CI passed.

@jerqi jerqi requested a review from zuston May 12, 2023 13:51
@jerqi jerqi changed the title [MINOR] refactor: Reduce the memory of usage in the ShuffleWriter [MINOR] refactor: Reduce the usage of memory in the ShuffleWriter May 12, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #877 (ffb1429) into master (9c26c1b) will increase coverage by 1.43%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #877      +/-   ##
============================================
+ Coverage     56.58%   58.02%   +1.43%     
+ Complexity     2176     2041     -135     
============================================
  Files           327      290      -37     
  Lines         15981    12661    -3320     
  Branches       1263     1185      -78     
============================================
- Hits           9043     7346    -1697     
+ Misses         6430     4909    -1521     
+ Partials        508      406     -102     

see 41 files with indirect coverage changes

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

Copy link
Member

@zuston zuston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Is it possible that WriteBuffer support ByteBuffer directly?

@jerqi
Copy link
Contributor Author

jerqi commented May 12, 2023

LGTM.

Is it possible that WriteBuffer support ByteBuffer directly?

This pr is doing similar thing #861

@jerqi
Copy link
Contributor Author

jerqi commented May 12, 2023

Merged to master. thanks @zuston

@jerqi jerqi merged commit 55ac4c2 into apache:master May 12, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants