GH-39377: [C++] IO: Reuse same buffer in CompressedInputStream #39807

mapleFU · 2024-01-26T14:47:29Z

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?

For compress_, allocate and reuse same buffer with kChunkSize (64KB), and reusing it
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: [C++] arrow::io::CompressedInputStream might allocate buffer to many times #39377

github-actions · 2024-01-26T14:47:59Z

⚠️ GitHub issue #39377 has been automatically assigned in GitHub to PR creator.

mapleFU · 2024-01-26T14:48:03Z

cc @pitrou @felipecrv

pitrou

This may be a good idea, but have you run any benchmarks?

pitrou · 2024-01-29T17:10:31Z

cpp/src/arrow/io/compressed.cc

  std::shared_ptr<ResizableBuffer> decompressed_;
+  std::shared_ptr<ResizableBuffer> decompressed_impl_;


Why are there two buffers suddently? What is the difference between the two?

It's not neccessary to maintain two buffer, I use it here for simplify the changes(previous impl set decompressed_ = nullptr to means decompressed is consumed). Seems this is not a good idea.

cpp/src/arrow/io/compressed.cc

cpp/src/arrow/io/compressed.h

mapleFU · 2024-01-30T14:42:42Z

have you run any benchmarks?

Currently not, let me find and run them

felipecrv · 2024-02-01T14:11:10Z

I really need to work on these stream classes before I can fee confident reviewing these optimizations.

pitrou · 2024-02-05T17:48:37Z

You should benchmark using a faster codec such as LZ4, if you really want to measure the overhead of CompressedInputStream.

mapleFU · 2024-02-05T17:51:39Z

(After changing supports_zero_copy_from_raw_ to const, my optimization would be a little faster. I'll dive into it tomorrow)

cpp/src/arrow/io/compressed_benchmark.cc

mapleFU · 2024-03-08T16:54:13Z

Sorry for delaying, I'm suffering from to much work this two weeks. I'll enhance this on weekend

pitrou · 2024-03-08T17:23:43Z

It's ok @mapleFU !

# Conflicts: # cpp/src/arrow/dataset/dataset_writer.cc

pitrou · 2024-03-27T08:10:11Z

@mapleFU I don't think there any existing C++ benchmarks that exercise CompressedInputStream (except the ones you have added). There may be Python benchmarks reading compressed CSV files, I'm not sure.

mapleFU · 2024-03-27T09:41:45Z

Aha, you're right

pitrou

+1. I unfortunately can't reproduce any speedup when reading a compressed CSV file from Python, but this looks potentially useful anyway.

pitrou · 2024-03-27T10:23:09Z

@github-actions crossbow submit -g cpp

mapleFU · 2024-03-27T10:25:36Z

Yeah, I'm use a CompressedInputStream to read csv/tsv/json... from remote object store. The previous code works well but this patch is some minor optimizations about it.

github-actions · 2024-03-27T10:25:42Z

Revision: 8ca0840

Submitted crossbow builds: ursacomputing/crossbow @ actions-371b6d0039

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14

conbench-apache-arrow · 2024-03-27T16:37:30Z

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit aae2557.

There were 8 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2024-03-27 11:58:44Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-11, scale_factor=1
and 6 more (see the report linked below)

The full Conbench report has more details. It also includes information about 10 possible false positives for unstable benchmarks that are known to sometimes produce them.

mapleFU · 2024-03-27T17:00:45Z

Don't understand the reason, would native read by R uses CompressedInputStream...?

pitrou · 2024-03-27T17:02:46Z

It's certainly unrelated.

kou · 2024-04-05T09:07:56Z

@github-actions crossbow submit test-r-rstudio-r-base-4.1-opensuse153

github-actions · 2024-04-05T09:20:33Z

Revision: 8ca0840

Submitted crossbow builds: ursacomputing/crossbow @ actions-7cf4daee98

Task	Status
test-r-rstudio-r-base-4.1-opensuse153

### Rationale for this change Previous pr ( #39807 ) remove std::move when returning value, however, it's not allowed in some old compilers ### What changes are included in this PR? add std::move for return, and add reason for that ### Are these changes tested? Should test by other ci ### Are there any user-facing changes? no * GitHub Issue: #41024 Authored-by: mwish <maplewish117@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

### Rationale for this change Previous pr ( apache#39807 ) remove std::move when returning value, however, it's not allowed in some old compilers ### What changes are included in this PR? add std::move for return, and add reason for that ### Are these changes tested? Should test by other ci ### Are there any user-facing changes? no * GitHub Issue: apache#41024 Authored-by: mwish <maplewish117@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

…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>

### Rationale for this change Previous pr ( apache#39807 ) remove std::move when returning value, however, it's not allowed in some old compilers ### What changes are included in this PR? add std::move for return, and add reason for that ### Are these changes tested? Should test by other ci ### Are there any user-facing changes? no * GitHub Issue: apache#41024 Authored-by: mwish <maplewish117@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

Reuse same buffer in CompressedInputStream

76341d5

github-actions bot added the Component: C++ label Jan 26, 2024

github-actions bot added the awaiting review Awaiting review label Jan 26, 2024

pitrou reviewed Jan 29, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 29, 2024

mapleFU added 2 commits January 30, 2024 22:18

Merge branch 'main' into compress-stream-update

119866a

Resolve comments

6f9d865

mapleFU added 2 commits January 31, 2024 01:05

fix stupid error

fb2bb77

Merge branch 'main' into compress-stream-update

584fc2e

Add benchmark for compress input

de100e9

This comment was marked as outdated.

Sign in to view

re-impl NonZeroCopyBufferReader

4a19061

pitrou reviewed Feb 5, 2024

View reviewed changes

cpp/src/arrow/io/compressed_benchmark.cc Outdated Show resolved Hide resolved

DatasetWriter: Check num_rows() before allocate batch

86a2a62

mapleFU added 2 commits March 18, 2024 10:45

Merge branch 'main' into compress-stream-update

344aca9

# Conflicts: # cpp/src/arrow/dataset/dataset_writer.cc

Update testing impl

7cb23a3

This comment was marked as outdated.

Sign in to view

Reducing the calling to ResizableBuffer::Resize

0521d1f

This comment was marked as outdated.

Sign in to view

mapleFU added 2 commits March 18, 2024 12:32

Fix lint

e50a404

add arrow/util/config.h for macro

fabb5b4

Merge branch 'main' into compress-stream-update

e50c3fc

pitrou added 2 commits March 27, 2024 11:02

Count decompressed bytes; naming nits

4c97871

Fix compile error

8ca0840

pitrou approved these changes Mar 27, 2024

View reviewed changes

pitrou merged commit aae2557 into apache:main Mar 27, 2024
36 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Mar 27, 2024

mapleFU deleted the compress-stream-update branch March 27, 2024 11:49

This comment was marked as outdated.

Sign in to view

kou mentioned this pull request Apr 5, 2024

[C++] Build failure with GCC 7.5.0 #41024

Closed

mapleFU mentioned this pull request Apr 5, 2024

GH-41024: [C++] IO: fixing compiling in gcc 7.5.0 #41025

Merged

This was referenced Apr 10, 2024

[C++] IO: Style enhancement for io::CompressedInputStream #41116

Closed

GH-41116: [C++] IO: enhance boundary checking in CompressedInputStream #41117

Merged

kou mentioned this pull request May 12, 2024

GH-41478: [C++] Clean up more redundant move warnings #41487

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-39377: [C++] IO: Reuse same buffer in CompressedInputStream #39807

GH-39377: [C++] IO: Reuse same buffer in CompressedInputStream #39807

mapleFU commented Jan 26, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Jan 26, 2024

mapleFU commented Jan 26, 2024

pitrou left a comment

pitrou Jan 29, 2024

mapleFU Jan 30, 2024

mapleFU commented Jan 30, 2024

felipecrv commented Feb 1, 2024

This comment was marked as outdated.

pitrou commented Feb 5, 2024

mapleFU commented Feb 5, 2024

mapleFU commented Mar 8, 2024

pitrou commented Mar 8, 2024

This comment was marked as outdated.

This comment was marked as outdated.

pitrou commented Mar 27, 2024

mapleFU commented Mar 27, 2024

pitrou left a comment

pitrou commented Mar 27, 2024

mapleFU commented Mar 27, 2024

github-actions bot commented Mar 27, 2024

conbench-apache-arrow bot commented Mar 27, 2024

mapleFU commented Mar 27, 2024

pitrou commented Mar 27, 2024

kou commented Apr 5, 2024 •

edited

Loading

This comment was marked as outdated.

github-actions bot commented Apr 5, 2024

		std::shared_ptr<ResizableBuffer> decompressed_;
		std::shared_ptr<ResizableBuffer> decompressed_impl_;

GH-39377: [C++] IO: Reuse same buffer in CompressedInputStream #39807

GH-39377: [C++] IO: Reuse same buffer in CompressedInputStream #39807

Conversation

mapleFU commented Jan 26, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jan 26, 2024

mapleFU commented Jan 26, 2024

pitrou left a comment

Choose a reason for hiding this comment

pitrou Jan 29, 2024

Choose a reason for hiding this comment

mapleFU Jan 30, 2024

Choose a reason for hiding this comment

mapleFU commented Jan 30, 2024

felipecrv commented Feb 1, 2024

This comment was marked as outdated.

pitrou commented Feb 5, 2024

mapleFU commented Feb 5, 2024

mapleFU commented Mar 8, 2024

pitrou commented Mar 8, 2024

This comment was marked as outdated.

This comment was marked as outdated.

pitrou commented Mar 27, 2024

mapleFU commented Mar 27, 2024

pitrou left a comment

Choose a reason for hiding this comment

pitrou commented Mar 27, 2024

mapleFU commented Mar 27, 2024

github-actions bot commented Mar 27, 2024

conbench-apache-arrow bot commented Mar 27, 2024

mapleFU commented Mar 27, 2024

pitrou commented Mar 27, 2024

kou commented Apr 5, 2024 • edited Loading

This comment was marked as outdated.

github-actions bot commented Apr 5, 2024

mapleFU commented Jan 26, 2024 •

edited by github-actions bot

Loading

kou commented Apr 5, 2024 •

edited

Loading