Skip to content

Reduce memory usage in writer by freeing unused buffers#23724

Merged
sdruzkin merged 1 commit intoprestodb:masterfrom
chenyangfb:orc_reset_chunk_supplier
Jun 13, 2025
Merged

Reduce memory usage in writer by freeing unused buffers#23724
sdruzkin merged 1 commit intoprestodb:masterfrom
chenyangfb:orc_reset_chunk_supplier

Conversation

@chenyangfb
Copy link
Contributor

@chenyangfb chenyangfb commented Sep 25, 2024

Description

Currently ChunkedSliceOutput keep buffers in bufferPool and usedBuffers, it never free those buffer, even with reset(), leads to extra memory usage and OOM.

This PR avoid those extra memory usage by freeing unused buffer in chunk supplier during reset. This behavior is controlled by resetOutputBuffer in OrcWriterOptions, and it's disabled by default.

Example ChunkedSliceOutput behaviour before and after the change
Assume first batch of output is large, it used 3.75M
after writing those output, and before we call reset()
the usedBuffers contains a list of buffer with increasing sizes (assuming min buffer is 256k, max buffer is 1M for illustration purpose)
256k, 512k, 1M, 1M, 1M

The second batch of output is smaller, it will only used 1.75M
after writing those output, and before we call reset()
usedBuffers have 256k, 512k, 1M,
bufferPool have 1M, 1M

before the change
we will keep all 5 buffers, and will never free them
after the change
we will keep the 3 buffer which are used (256k, 512k, 1M) and free up the 2 buffer which are unused (1M, 1M)

In other word, previous behaviour only scale up number of buffers, new behaviour also can scale down number of buffer based on memory usage

Impact

Reduce memory usage in writer.

Test Plan

Tested with Spark workload

Example output showing unused buffer getting freed after the change

Reset unused buffers, used 17 chunks (8387584 bytes), unused 65 chunks (68157440 bytes)
Reset unused buffers, used 82 chunks (76545024 bytes), unused 4 chunks (4194304 bytes)
== RELEASE NOTES ==
General change
* Improve memory usage in writer by feeing unused buffers.

@chenyangfb chenyangfb force-pushed the orc_reset_chunk_supplier branch from 01405f7 to 3f8ec38 Compare September 27, 2024 23:23
@chenyangfb chenyangfb force-pushed the orc_reset_chunk_supplier branch from 3f8ec38 to 7cab405 Compare November 3, 2024 18:43
@chenyangfb chenyangfb force-pushed the orc_reset_chunk_supplier branch 4 times, most recently from a6fd141 to 2d5d698 Compare April 14, 2025 16:45
@chenyangfb chenyangfb changed the title Free unused buffer in chunk supplier during reset Reduce memory usage in writer by freing unused buffers Apr 14, 2025
@chenyangfb chenyangfb changed the title Reduce memory usage in writer by freing unused buffers Reduce memory usage in writer by frefing unused buffers Apr 14, 2025
@chenyangfb chenyangfb changed the title Reduce memory usage in writer by frefing unused buffers Reduce memory usage in writer by freeing unused buffers Apr 14, 2025
@chenyangfb chenyangfb marked this pull request as ready for review April 14, 2025 17:44
@chenyangfb chenyangfb requested review from a team and sdruzkin as code owners April 14, 2025 17:44
@chenyangfb chenyangfb requested a review from jaystarshot April 14, 2025 17:44
@chenyangfb chenyangfb force-pushed the orc_reset_chunk_supplier branch from 2d5d698 to 68885be Compare April 18, 2025 23:51
@sdruzkin
Copy link
Collaborator

sdruzkin commented Apr 20, 2025

Memory allocation is expensive and we added the pool for this reason. Please add performance benchmark results for a case comparing writing a 500MB file with 5-7 columns with the new option enabled and disabled.

@chenyangfb chenyangfb force-pushed the orc_reset_chunk_supplier branch 2 times, most recently from 76c15f2 to e78aef2 Compare May 5, 2025 21:54
@steveburnett
Copy link
Contributor

Thanks for the release note! Just a nit or two suggested.

== RELEASE NOTES ==

General Changes
* Improve memory usage in writer by freeing unused buffers.

bufferPool.clear();
System.setProperty("RESET_OUTPUT_BUFFER", "RESET_OUTPUT_BUFFER");
}
bufferPool.addAll(0, usedBuffers);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to save on the memory, do you really want to keep the last batch of chunks? I suppose you don't.

usedBuffers.stream().mapToInt(b -> b.length).sum(),
bufferPool.size(),
bufferPool.stream().mapToInt(b -> b.length).sum());
bufferPool.clear();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a tricky place. If you look at the ChunkSupplier.get method you will see that ChunkSupplier scales up chunks from 256 bytes all the way to 16MB when it needs to produce a new chunk. If you reset the pool and start producing new chunks you will eventually end up only with big 16MB new chunks. Which is probably opposite of your goal.

I don't know what is a best strategy here, because it's really depends on the data shapes, but I see a few options:

  1. Have chunk supplier that does not have a buffer, and alway produces smaller fixed size chunks. Will perform best in terms of overhead, but will regress for small streams.
  2. Have chunk supplier that does not have a buffer, and alway produces scaled up chunks with reset resetting the current size to min size. Good middle ground option.
  3. Have chunk supplier that does not have a buffer, and alway produces scaled up chunks with reset resetting the current size to min size AND using smaller max chunk size. Will perform even better in terms of overhead, won't regress as much as 1 for small streams.

@chenyangfb chenyangfb force-pushed the orc_reset_chunk_supplier branch from e78aef2 to 3fb5783 Compare June 6, 2025 16:07
@tdcmeehan tdcmeehan added the from:Meta PR from Meta label Jun 9, 2025
@prestodb-ci
Copy link
Contributor

Saved that user @chenyangfb is from Meta

@chenyangfb
Copy link
Contributor Author

Discussed with Sergii offline, ran Validation Service (Vader), results looks good, ~3.5k successful samples without failures https://fburl.com/scuba/dwrf_reader_checksum/crm55ef8

@sdruzkin sdruzkin merged commit f137ef2 into prestodb:master Jun 13, 2025
97 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:Meta PR from Meta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants