GH-39122: [C++][Parquet] Optimize FLBA record reader

[pitrou]

Rationale for this change

The FLBA implementation of RecordReader is suboptimal:

• it doesn't preallocate the output array
• it reads the decoded validity bitmap one bit at a time and recreates it, one bit at a time

What changes are included in this PR?

Optimize the FLBA implementation of RecordReader so as to avoid the aforementioned inefficiencies.
I did a quick-and-dirty benchmark on a Parquet file with two columns:

• column 1: uncompressed, PLAIN-encoded, FLBA<3> with no nulls
• column 2: uncompressed, PLAIN-encoded, FLBA<3> with 25% nulls

With git main, the file can be read at 465 MB/s. With this PR, the file can be read at 700 MB/s.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

• Closes: [C++] [Parquet] FLBA reader does not pre-reserve memory #39122

[pitrou]

This is an alternative to #39120. @Hattonuri could you try it out?

[pitrou]

@ursabot please benchmark

[ursabot]

Benchmark runs are scheduled for commit e0b5609. 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]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: e0b5609

Submitted crossbow builds: ursacomputing/crossbow @ actions-c4f6660a25

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-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

[mapleFU]

+1

[conbench-apache-arrow]

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

There were 5 benchmark results indicating a performance regression:

• Pull Request Run on ursa-thinkcentre-m75q at 2023-12-07 19:12:42Z
  • BM_ArrowBinaryDict (C++) with params=EncodeDictDirectInt8/1048576, source=cpp-micro, suite=parquet-encoding-benchmark
  • BM_ArrowBinaryDict (C++) with params=EncodeDictDirectInt64/1048576, source=cpp-micro, suite=parquet-encoding-benchmark
• and 3 more (see the report linked below)

The full Conbench report has more details.

[Hattonuri]

Comparing to my pull request you have 5% speedup

But I accidentally found that between this commit and 2dcee3f

There was a commit that increased the number of page faults by two times

In this commit i have

Command being timed: "./parquet_playground"
User time (seconds): 132.78
System time (seconds): 101.82
Percent of CPU this job got: 497%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:47.18
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 654652
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 56145278
Voluntary context switches: 69281
Involuntary context switches: 716
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0

And before it was

Command being timed: "./parquet_playground"
User time (seconds): 153.61
System time (seconds): 44.16
Percent of CPU this job got: 506%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:39.06
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 707356
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 27169121
Voluntary context switches: 46224
Involuntary context switches: 570
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0

I guess your pull request optimizes User time. But as we can see page faults increased from 27169121 to 56145278 and system time increased by 2.5 times. Also context switches increased by 1.5 times.
In my "playground" i just create acero ScanNode + SinkNode and use MakeGeneratorReader calls to Next() reading ListArrays of Decimal128 with length <= 20 (almost always 20)
Fragment readahead is 2, batch readahead is 1 and require_sequenced_output is enabled with default backpressure

[pitrou]

I don't think we can infer anything from that number of page faults, except if it's stable otherwise.

[Hattonuri]

When capturing perf record -D 1000 -e minor-faults -g --freq=max --call-graph=dwarf,32768
this is old one

and this is new one

Big difference is seen in ZSTD_decompressSequences_bmi2 - it started to cause 8 times more minor faults
Also CreateBuffer of started to take much time. About RawBytesToDecimal probably it also started to take much time or it is part of transfering to decimal as well as read values spaced

[mapleFU]

🤔Though benchmark using SystemAllocator is a bit weird, should we use something like jemalloc?

Also ZSTD_decompressSequences_bmi2 shouldn't introduce any differences if the page we read doesn't changed, except we read more pages?

[pitrou]

Big difference is seen in ZSTD_decompressSequences_bmi2 - it started to cause 8 times more minor faults

Are you just comparing percentages? This is silly, isn't it?

[Hattonuri]

🤔Though benchmark using SystemAllocator is a bit weird, should we use something like jemalloc?

Also ZSTD_decompressSequences_bmi2 shouldn't introduce any differences if the page we read doesn't changed, except we read more pages?

i compile all my project with tcmalloc

Are you just comparing percentages? This is silly, isn't it?

I think that it's not in that case because page faults increased and zstd increased - so we can still see that that it is changed

I will try to binary search breaking commit. But I think that it needs another issue because we see that the problem is not with that PR

[pitrou]

I will try to binary search breaking commit.

No "breakage" has been proven, though. You've found out a variation in number of page faults. So what? Are you sure the system was quiet? Are you sure the number of page faults is otherwise a stable quantity?

That said, it will be interesting to see the results of your investigation.

[Hattonuri]

Are you sure the system was quiet?

It did not affect the measurement because
I did measurement in a way like that: build + perf new, build + perf old, build + perf new ... and 5 round of like that

Are you sure the number of page faults is otherwise a stable quantity?

Yes, the results are stable. page faults number repeats

[Hattonuri]

About investigation - i mean that i will put in another issue the results because they repeat stably - so i can find the reason with binary search

[wgtmac]

BTW, this patch reminds me of this ancient PR: #14353

[conbench-apache-arrow]

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

There were 8 benchmark results indicating a performance regression:

• Commit Run on ursa-i9-9960x at 2023-12-09 17:15:10Z
  • file-read (R) with compression=uncompressed, dataset=fanniemae_2016Q4, file_type=feather, language=R, output_type=dataframe
  • file-read (R) with compression=lz4, dataset=nyctaxi_2010-01, file_type=feather, language=R, output_type=table
• and 6 more (see the report linked below)

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