Skip to content

Conversation

@wesm
Copy link
Member

@wesm wesm commented Sep 4, 2019

A handful of things here:

  • Using preallocation and UnsafeAppend on the primary binary read path
  • Changed Parquet decode APIs to decode into a helper data structure to avoid the extra machinery of ChunkedBinaryBuilder. These APIs are pseudopublic (for testing purposes) and not exposed to the user, so this doesn't affect any public APIs

This produces about 10% net benefit in a holistic benchmark script from Python. The microbenchmarks in parquet-encoding-benchmark are much more clear

before

BM_ArrowBinaryPlain/DecodeArrow_Dense/1024              20204 ns      20205 ns      34568   307.562MB/s
BM_ArrowBinaryPlain/DecodeArrow_Dense/4096              81111 ns      81111 ns       8581   295.352MB/s
BM_ArrowBinaryPlain/DecodeArrow_Dense/32768            622801 ns     622797 ns       1102   300.966MB/s
BM_ArrowBinaryPlain/DecodeArrow_Dense/65536           1245664 ns    1245663 ns        561   301.988MB/s
BM_ArrowBinaryPlain/DecodeArrowNonNull_Dense/1024       17865 ns      17865 ns      39028   347.839MB/s
BM_ArrowBinaryPlain/DecodeArrowNonNull_Dense/4096       71837 ns      71835 ns       9604   333.492MB/s
BM_ArrowBinaryPlain/DecodeArrowNonNull_Dense/32768     564087 ns     564075 ns       1248   332.298MB/s
BM_ArrowBinaryPlain/DecodeArrowNonNull_Dense/65536    1123738 ns    1123722 ns        611   334.758MB/s

after

BM_ArrowBinaryPlain/DecodeArrow_Dense/1024               5922 ns       5923 ns     115651   1049.24MB/s
BM_ArrowBinaryPlain/DecodeArrow_Dense/4096              35340 ns      35340 ns      19920   677.887MB/s
BM_ArrowBinaryPlain/DecodeArrow_Dense/32768            319888 ns     319882 ns       2194   585.968MB/s
BM_ArrowBinaryPlain/DecodeArrow_Dense/65536            642640 ns     642640 ns       1100   585.358MB/s
BM_ArrowBinaryPlain/DecodeArrowNonNull_Dense/1024        6568 ns       6568 ns     104715   946.191MB/s
BM_ArrowBinaryPlain/DecodeArrowNonNull_Dense/4096       30890 ns      30890 ns      22661    775.53MB/s
BM_ArrowBinaryPlain/DecodeArrowNonNull_Dense/32768     257427 ns     257426 ns       2711   728.135MB/s
BM_ArrowBinaryPlain/DecodeArrowNonNull_Dense/65536     516614 ns     516600 ns       1350   728.174MB/s

The dictionary decoding case is unchanged; this should be optimized separately.

@wesm
Copy link
Member Author

wesm commented Sep 4, 2019

@pitrou @bkietz I changed the BufferBuilder growth strategy from 1.5x to 2x in this patch. I can break that out into a separate patch and run benchmarks if you'd like -- it's a bit weird to make that kind of change in passing here.

@pitrou
Copy link
Member

pitrou commented Sep 4, 2019

Yes, better use a separate PR. When I tried locally the benchmark numbers were surprisingly unconvincing.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@ea309dd). Click here to learn what that means.
The diff coverage is 92.82%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5268   +/-   ##
=========================================
  Coverage          ?   89.22%           
=========================================
  Files             ?      750           
  Lines             ?   108428           
  Branches          ?        0           
=========================================
  Hits              ?    96744           
  Misses            ?    11684           
  Partials          ?        0
Impacted Files Coverage Δ
cpp/src/arrow/buffer_builder.h 100% <ø> (ø)
cpp/src/parquet/encoding_test.cc 100% <100%> (ø)
cpp/src/parquet/encoding.h 97.05% <100%> (ø)
cpp/src/arrow/array/builder_binary.cc 90.42% <100%> (ø)
cpp/src/parquet/column_reader.cc 89% <100%> (ø)
cpp/src/parquet/encoding.cc 91.69% <90.6%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea309dd...a4eab7d. Read the comment docs.

@wesm
Copy link
Member Author

wesm commented Sep 5, 2019

+1. I'm leaving the JIRA open because the performance issue is not resolved

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

Successfully merging this pull request may close these issues.

3 participants