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

Huge memory taken for each field when exporting #1240

Open
LouisClt opened this issue Aug 30, 2022 · 14 comments
Open

Huge memory taken for each field when exporting #1240

LouisClt opened this issue Aug 30, 2022 · 14 comments

Comments

@LouisClt
Copy link

Hello,
Using arrow adapter, I became aware that the memory (RAM) footprint of the export (exporting an orc file) was very huge for each field. For instance, exporting a table with 10000 fields can take up to 30Go, even if there is only 10 records.
Even for 100 fields, that could take 100Mo+.
The "issue" seems to be coming from here :

1 * 1024 * 1024,

When we create a writer with the "createWriter" (

orc/c++/src/Writer.cc

Lines 681 to 684 in 432a7aa

std::unique_ptr<Writer> createWriter(
const Type& type,
OutputStream* stream,
const WriterOptions& options) {
), a stream (compressor) is created for each field. As we allocate a Buffer of 1 * 1024 *1024 we get as a minimum 1Mo additionnal size taken in memory for each field.

Is there a reason the BufferedOutputStream initial capacity is that high ? I circumvented my problem by lowering it to 1Ko (it didn't change much the performance according to my testing, but it may depend on usecases). Could it be envisaged to put a global variable (or static one) to parametrize this to allow changing this hard coded parameter ?
Thanks

@dongjoon-hyun
Copy link
Member

cc @wgtmac , @stiga-huang , @coderex2522

@coderex2522
Copy link
Contributor

@LouisClt To support the zero-copy mechanism, class BufferedOutputStream will have an internal data buffer. And the default capacity of the internal data buffer is 1MB. This default capacity size should be able to be modified, but here's a hint that if the buffer capacity is set too small, it may cause the buffer to expand and trigger memcpy function frequently.

@wgtmac
Copy link
Member

wgtmac commented Sep 13, 2022

We may replace the DataBuffer by a new Buffer implementation with a much smarter memory management to automatically grow and shrink its size according to actual usage. This management can happen on the column basis.

@LouisClt
Copy link
Author

Thanks everyone for your answers. I understand the possible performances issues linked with lowering too much the size of the buffer (on my testing it was OK in my case though).
I think the solution given by @wgtmac would be fine for me, and better than passing by global variables, if it is feasible.

@wgtmac
Copy link
Member

wgtmac commented Sep 27, 2022

I have created a JIRA to track the progress: https://issues.apache.org/jira/browse/ORC-1280

@coderex2522
Copy link
Contributor

coderex2522 commented Sep 27, 2022

@dongjoon-hyun @wgtmac @LouisClt I will follow up on this issues(ORC-1280) and implement a much smarter memory management.

@dongjoon-hyun
Copy link
Member

Thank you, @coderex2522 .

dongjoon-hyun pushed a commit that referenced this issue Oct 26, 2022
…OutputStream

### What changes were proposed in this pull request?
This PR can solve the huge memory taken by BufferedOutputStream and refactor the write data logic in class CompressionBase.

### Why are the changes needed?
This patch use BlockBuffer to replace DataBuffer  of class BufferedOutputStream in order to solve the [issue](#1240).

### How was this patch tested?
The UTs in TestBufferedOutputStream.cc and TestCompression.cc can cover this patch. Add the TestBlockBuffer.write_to UT.

Closes #1275 from coderex2522/ORC-1280-PART2.

Authored-by: coderex2522 <rex6817@gmail.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@LouisClt
Copy link
Author

Hello, it seems there were commits referencing this issue. Is this issue now fixed ?

@wgtmac
Copy link
Member

wgtmac commented Jan 24, 2023

Hello, it seems there were commits referencing this issue. Is this issue now fixed ?

@LouisClt Thanks for your follow-up.

We have implemented a block-based buffer called BlockBuffer (by @coderex2522) and used it to replace the output buffer in the CompressionStream. It can decrease the memory footprint to some extent.

IMO, the next step is to use it to replace the input buffer of the CompressionStream which has the size of compressionBlockSize per stream.

@wgtmac
Copy link
Member

wgtmac commented Feb 3, 2023

Hello, it seems there were commits referencing this issue. Is this issue now fixed ?

@LouisClt Thanks for your follow-up.

We have implemented a block-based buffer called BlockBuffer (by @coderex2522) and used it to replace the output buffer in the CompressionStream. It can decrease the memory footprint to some extent.

IMO, the next step is to use it to replace the input buffer of the CompressionStream which has the size of compressionBlockSize per stream.

To be precise, the rawInputBuffer of every CompressionStream is fixed to the compression block size which is 1M by default. Writer with many columns will suffer from large memory footprint and nothing can be done to alleviate it.

I have created a JIRA to track it: https://issues.apache.org/jira/browse/ORC-1365

cc @coderex2522

@LouisClt
Copy link
Author

LouisClt commented Feb 6, 2023

Thanks for your reply @wgtmac and the implementation of the BlockBuffer.
I'll wait for the replacement of the rawInputBuffer by the BlockBuffer in every compression stream then. Do you think it will take long ?

@dongjoon-hyun
Copy link
Member

Hi, @LouisClt . FYI, according to the Apache ORC release cycle, newly developed features will be delivered via v1.9.0 on September 2023 (if they are merged to Apache ORC before.)

@LouisClt
Copy link
Author

LouisClt commented Feb 6, 2023

Understood, and thanks for your answer !

@luffy-zh
Copy link
Contributor

luffy-zh commented Feb 7, 2023

I will work on it.

cxzl25 pushed a commit to cxzl25/orc that referenced this issue Jan 11, 2024
…OutputStream

### What changes were proposed in this pull request?
This PR can solve the huge memory taken by BufferedOutputStream and refactor the write data logic in class CompressionBase.

### Why are the changes needed?
This patch use BlockBuffer to replace DataBuffer  of class BufferedOutputStream in order to solve the [issue](apache#1240).

### How was this patch tested?
The UTs in TestBufferedOutputStream.cc and TestCompression.cc can cover this patch. Add the TestBlockBuffer.write_to UT.

Closes apache#1275 from coderex2522/ORC-1280-PART2.

Authored-by: coderex2522 <rex6817@gmail.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
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

No branches or pull requests

5 participants