GH-38432: [C++][Parquet] Try to fix performance regression in the DictByteArrayDecoderImpl

[mapleFU]

Rationale for this change

Do some changes mentioned in #38432

I believe this might fix #38577

Problem1:

The BinaryHelper might call Prepare() and Prepare(estimated-output-binary-length) for data. This might because:

1. For Plain Encoding ByteArray, the len_ is similar to the data-page size, so Reserve is related.
2. For Dict Encoding. The Data Page is just a RLE encoding Page, it's len_ might didn't directly related to output-binary.

Problem2:

Prepare using ::arrow::kBinaryMemoryLimit as min-value, we should use this->chunk_space_remaining_.

Problem3:

std::optional<int64_t> is hard to optimize for some compilers

What changes are included in this PR?

Mention the behavior of BinaryHelper. And trying to fix it.

Are these changes tested?

No

Are there any user-facing changes?

Regression fixes

• Closes: [C++] Parquet reading performance regressions #38432

[mapleFU]

@rok @pitrou I try to figure out how the regression happened, and add comment for it. Would you mind take a look?

[rok]

Thanks for working on this @mapleFU !

[mapleFU]

@ursabot please benchmark

[ursabot]

Benchmark runs are scheduled for commit c75517b. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

[conbench-apache-arrow]

Thanks for your patience. Conbench analyzed the 5 benchmarking runs that have been run so far on PR commit c75517b.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[wgtmac]

How should we interpret the benchmark report to verify the regression is fixed?

[jorisvandenbossche]

How should we interpret the benchmark report to verify the regression is fixed?

The problem is that the machine ursa-i9-9960x is currently not running (not fully sure why), and it's on this one that the affected benchmark is run (see #38437 (comment) in the previous PR where I linked to the relevant in the benchmark run back then)

[mapleFU]

Would you mind help re-run or trigger that running? @jorisvandenbossche 🤔

[mapleFU]

@ursabot please benchmark

[ursabot]

Benchmark runs are scheduled for commit f74b4c1. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

[pitrou]

@mapleFU Can you please make your PR message more descriptive? It should help understand what this PR is about.

[pitrou]

Also, I don't understand why this would fix dictionary decoding, and why this would be ok for non-dictionary decoding. This lacks a serious analysis IMHO.

[conbench-apache-arrow]

Thanks for your patience. Conbench analyzed the 5 benchmarking runs that have been run so far on PR commit f74b4c1.

There were 6 benchmark results indicating a performance regression:

• Pull Request Run on ursa-thinkcentre-m75q at 2023-11-20 12:17:10Z
  • compareBenchmark (Java) with source=java-micro, suite=arrow.memory.util.ArrowBufPointerBenchmarks
  • ShortVectorInsertAtEnd (C++) with params=<SMALL_VECTOR(int)>, source=cpp-micro, suite=arrow-small-vector-benchmark
• and 4 more (see the report linked below)

The full Conbench report has more details.

[mapleFU]

Also, I don't understand why this would fix dictionary decoding, and why this would be ok for non-dictionary decoding. This lacks a serious analysis IMHO.

This is easy because:

1. len_ is the size of Page's data payload
2. PlainByteArrayDecoder's len_ is similiar to the final result(each record might have a length). But for dictionary, the len_ might be unrelated to the ByteArray size, which could make the Program reserve unneccessary memory. Evenmore, it doesn't decrease len_ after each decode.

[mapleFU]

@ursabot please benchmark

[ursabot]

Benchmark runs are scheduled for commit 60cdb80. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

[mapleFU]

@jorisvandenbossche I think problem is fixed, aha

See https://conbench.ursa.dev/compare/runs/f12445ad2fb84f32a924a483ab75e0db...a478ff692d97498eade4936be3d54fcf/

I'll merge this later if no negative comments

[pitrou]

What difference does it make to have a separate method?

[mapleFU]

https://godbolt.org/z/5bf1K55a5

Found it might not optimized with hot path. Prepare is not in hot path, but PrepareNextInput is.

[pitrou]

@mapleFU Please don't merge a change if you don't understand how it works.

[pitrou]

Note that @jorisvandenbossche above didn't test the same changes...

[mapleFU]

#38784 (comment)

@pitrou I think the critical reason is here. The compiler is hard to optimize:

Status PrepareNextInput(int64_t, std::optional<int64_t> = std::nullopt)

The function above is called frequently in a hot-path. Another two reason is about the #38577

[mapleFU]

I've try benchmark here: #38784 (comment)

[pitrou]

@pitrou I think the critical reason is here. The compiler is hard to optimize:

Are you able to measure a difference between those two versions of the code? Is there a micro-benchmark?

[mapleFU]

Sure, please wait for a minute.

Also I'm trying on LLVM-17, maybe I can simplify the case using godbolt

[conbench-apache-arrow]

Thanks for your patience. Conbench analyzed the 6 benchmarking runs that have been run so far on PR commit 60cdb80.

There were 2 benchmark results indicating a performance regression:

• Pull Request Run on arm64-m6g-linux-compute at 2023-11-24 16:29:21Z

  • IntegerFormatting (C++) with params=, source=cpp-micro, suite=arrow-value-parsing-benchmark

• Pull Request Run on arm64-t4g-linux-compute at 2023-11-24 16:33:14Z

  • ExecuteScalarExpressionBaseline (C++) with params=/rows_per_batch:1000000/real_time/threads:8, source=cpp-micro, suite=arrow-acero-expression-benchmark

The full Conbench report has more details.

[mapleFU]

The current benchmark is out. @jorisvandenbossche would you mind take a look?

@pitrou https://godbolt.org/z/5bf1K55a5 I use same compiler-options, and the std::optional one seems contruct more instr in hot path?

[jorisvandenbossche]

Strangely on the latest version I don't directly see any effect locally, but, at least the online benchmarks now seem to confirm some improvement (several of the similar benchmarks are consistent): for example https://conbench.ursa.dev/compare/benchmark-results/0655f5af97857ba780005944ff57195f...06560d57f1f772cc800009452aab4863/

[mapleFU]

The difference between current version and 13.0.0 version is it call more BinaryBuilder->Reserve(), I don't think it would make performance worse, so I revert some change in 60cdb80 . Maybe we can considering this later.

This patch also separate PrepareNextInput. Maybe it's a compiler specific problem, I've check a similar case in quick-bench, it doesn't benfit the performance if we remove std::optional<> as argument in gcc-12.3 with C++17. However benchmark shows that the performance has benefits. Maybe we can ask some C++ experts for help.

You can decide how to handle this patch later, maybe I'm becoming crazy because spending long time but don't know why previously, at least we found that:

1. Reserve might related to the problem, but in ursabot it don't affect the performance a lot
2. PrepareInput in the patch might make compiler doesn't do some optimizations in ursa bot benchmark

[mapleFU]

Will wait util monday night to see any futher request for review

[mapleFU]

@pitrou Would you like to checkin this?

[conbench-apache-arrow]

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

There was 1 benchmark result indicating a performance regression:

• Commit Run on ursa-i9-9960x at 2023-11-27 21:41:51Z
  • partitioned-dataset-filter (R) with dataset=dataset-taxi-parquet, language=R, query=dims

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