-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[C++] arrow::io::CompressedInputStream might allocate buffer to many times #39377
Comments
Wait... does I wonder if there is something to learn from the zero-copy stream design implemented in protocolbuffers https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/io/zero_copy_stream.h |
Status Resize(const int64_t new_size, bool shrink_to_fit = true) override {
if (ARROW_PREDICT_FALSE(new_size < 0)) {
return Status::Invalid("Negative buffer resize: ", new_size);
}
uint8_t* ptr = mutable_data();
if (ptr && shrink_to_fit && new_size <= size_) {
// Buffer is non-null and is not growing, so shrink to the requested size without
// excess space.
int64_t new_capacity = bit_util::RoundUpToMultipleOf64(new_size);
if (capacity_ != new_capacity) {
// Buffer hasn't got yet the requested size.
RETURN_NOT_OK(pool_->Reallocate(capacity_, new_capacity, alignment_, &ptr));
data_ = ptr;
capacity_ = new_capacity;
}
} else {
RETURN_NOT_OK(Reserve(new_size));
}
size_ = new_size;
return Status::OK();
} @felipecrv |
Do you mean GzipStream here: https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/io/gzip_stream.h ? I think it's logic is similiar ( https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/io/gzip_stream.cc#L97 ). It read from Zero-copy stream, having a 64K internal buffer, arrow uses 1MiB. I'm sure it will keep 64KB memory util end We might be a bit complex when
|
You're right. The implementation is not a |
Late reply to this, but you were looking at the system memory pool. Look for example at the mimalloc memory pool, and you'll see that it reallocates: arrow/cpp/src/arrow/memory_pool.cc Lines 419 to 420 in 24feab0
Same for the jemalloc memory pool here: arrow/cpp/src/arrow/memory_pool_jemalloc.cc Lines 111 to 113 in 24feab0
|
### Rationale for this change This patch reuses the same buffer in `CompressedInputStream`. It includes the `decompress_` and `compress_` buffer ### What changes are included in this PR? 1. For `compress_`, allocate and reuse same buffer with `kChunkSize` (64KB), and reusing it 2. For `decompress_`, reusing a same buffer (mostly 1MB) without continues `Reallocate` In the worst case, `decompress_` might hold a large buffer. ### Are these changes tested? Already ### Are there any user-facing changes? `CompressedInputStream` might has larger buffer * Closes: #39377 Lead-authored-by: mwish <maplewish117@gmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Co-authored-by: mwish <anmmscs_maple@qq.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
…pache#39807) ### Rationale for this change This patch reuses the same buffer in `CompressedInputStream`. It includes the `decompress_` and `compress_` buffer ### What changes are included in this PR? 1. For `compress_`, allocate and reuse same buffer with `kChunkSize` (64KB), and reusing it 2. For `decompress_`, reusing a same buffer (mostly 1MB) without continues `Reallocate` In the worst case, `decompress_` might hold a large buffer. ### Are these changes tested? Already ### Are there any user-facing changes? `CompressedInputStream` might has larger buffer * Closes: apache#39377 Lead-authored-by: mwish <maplewish117@gmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Co-authored-by: mwish <anmmscs_maple@qq.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
Describe the enhancement requested
in
::arrow::io::CompressedInputStream::Impl
, thedecompressed_buffer_
is initialized as 1MB, and would:Resize
to release the memory if possible ( https://github.com/apache/arrow/blob/main/cpp/src/arrow/io/compressed.cc#L303 )Can we avoid re-allocating the buffer frequently, trying to keep 1MB data here?
Component(s)
C++
The text was updated successfully, but these errors were encountered: