ARROW-4341: [C++] Refactor Primitive builders and BooleanBuilder to use TypedBufferBuilder<T> #3575

bkietz · 2019-02-06T20:02:54Z

This reduces code duplication.

bkietz · 2019-02-06T20:32:33Z

Hmmm, this is a performance regression.

Before:

BM_BuildPrimitiveArrayNoNulls/repeats:2_mean                 365772 us     365779 us          1     2.291GB/s
BM_BuildVectorNoNulls/repeats:2_mean                         555089 us     555097 us          1   922.362MB/s
BM_BuildBooleanArrayNoNulls/repeats:2_mean                   121138 us     121139 us          6   4.12748GB/s
BM_BuildAdaptiveIntNoNulls/repeats:2_mean                     13406 us      13406 us         52   4.66192GB/s
BM_BuildAdaptiveIntNoNullsScalarAppend/repeats:3_mean         28445 us      28445 us         25   2.19714GB/s
BM_BuildAdaptiveUIntNoNulls/repeats:2_mean                    26330 us      26331 us         27   4.74726GB/s
BM_BuildAdaptiveUIntNoNullsScalarAppend/repeats:2_mean        27448 us      27448 us         25   2.27696GB/s
BM_BuildInt64DictionaryArrayRandom/repeats:2_mean               136 us        136 us       5063   561.917MB/s
BM_BuildInt64DictionaryArraySequential/repeats:2_mean           125 us        125 us       5606   611.482MB/s
BM_BuildInt64DictionaryArraySimilar/repeats:2_mean              126 us        126 us       5546   605.348MB/s
BM_BuildStringDictionaryArray/repeats:2_mean                    317 us        317 us       2209   315.468MB/s

After:

BM_BuildPrimitiveArrayNoNulls/repeats:2_mean                 417623 us     417631 us          1   1.75068GB/s
BM_BuildVectorNoNulls/repeats:2_mean                         555594 us     555601 us          1   921.525MB/s
BM_BuildBooleanArrayNoNulls/repeats:2_mean                   730000 us     730006 us          1   701.364MB/s
BM_BuildAdaptiveIntNoNulls/repeats:2_mean                     13394 us      13394 us         52   4.66611GB/s
BM_BuildAdaptiveIntNoNullsScalarAppend/repeats:3_mean         28451 us      28451 us         25    2.1967GB/s
BM_BuildAdaptiveUIntNoNulls/repeats:2_mean                    26312 us      26313 us         27   4.75047GB/s
BM_BuildAdaptiveUIntNoNullsScalarAppend/repeats:2_mean        27457 us      27457 us         25    2.2762GB/s
BM_BuildInt64DictionaryArrayRandom/repeats:2_mean               139 us        139 us       5040   549.399MB/s
BM_BuildInt64DictionaryArraySequential/repeats:2_mean           126 us        126 us       5570   606.613MB/s
BM_BuildInt64DictionaryArraySimilar/repeats:2_mean              129 us        129 us       5406   589.165MB/s
BM_BuildStringDictionaryArray/repeats:2_mean                    319 us        319 us       2193   313.216MB/s

wesm · 2019-02-06T21:47:47Z

cpp/src/arrow/array/builder_primitive.h

This is fishy

wesm · 2019-02-06T21:48:07Z

cpp/src/arrow/array/builder_primitive.h

wesm · 2019-02-06T21:48:13Z

cpp/src/arrow/array/builder_primitive.h

wesm · 2019-02-06T21:49:28Z

cpp/src/arrow/buffer-builder.h

This is a lot more CPU instructions than what it was doing before AFAICT

xhochy · 2019-02-08T21:33:11Z

Rebased.

wesm · 2019-02-14T06:13:25Z

BooleanBuilder seems to be more than 4 times slower with this patch

after

BM_BuildBooleanArrayNoNulls/repeats:2                        789455 us     789468 us          1   648.538MB/s

before

BM_BuildBooleanArrayNoNulls/repeats:2                        172299 us     172302 us          4   2.90189GB/s

Based on this evidence the iterator based unrolled bit generator vectorizes poorly. I just pushed changes be8442f reverting some of these changes (but still using TypedBufferBuilder<bool>, so better than before), as well as an option to disable tracking the false count, which we don't care about. For the record, disabling the false count only improved the benchmark from 650 MB/s to 1 GB/s, far less than the original 3GB/s. The compiler just isn't generating very good code for the CPU

If these changes are acceptable I suggest that we merge this patch (once the build is passing) and leave this be for now.

wesm · 2019-02-14T06:14:24Z

With my changes the perf seems slightly improved from the baseline

BM_BuildBooleanArrayNoNulls/repeats:2                        163077 us     162609 us          4   3.07486GB/s

wesm · 2019-02-14T06:21:56Z

It seems that there are symbol visibility problems on Windows caused by the PrimitiveBuilder / NumericBuilder collapse, and valgrind problems additionally. I would say to limit how much additional time (say 2 hours or less) you spend on this patch as there's a lot of feature work and bug fixes in the 0.13 backlog.

wesm · 2019-02-14T06:35:07Z

I moved the entire NumericBuilder<T> implementation to the header file. I think this should make the linker problems go away. I don't expect an application to have a ton of instantiations of these templates so I don't know that it's worth stressing about code size for these until it can be demonstrated to be a problem. The size of arrow-array-test in a release build increased by about 3%.

only zero padding on Finish()

…track falses. Restore faster code from before this patch for appending C arrays and vector<bool> Change-Id: I01d2d8e87db520a5c38f892a58e86d7a645e8877

…ity concerns Change-Id: I5cddcfde0880f5a1af4cc10a3740a186064e85ea

Change-Id: I749eab94c580775a9faf48b6baa0dabf5dc39111

Change-Id: I6fbf60477bd95d828ff14c54a124c0d079065bfb

Change-Id: Ic32f48c3cd5c5d34494a2e515848e5249c1ca56f

wesm · 2019-02-15T06:33:56Z

I reverted some changes and think I fixed the valgrind / Windows / Python failures

Change-Id: I9f24a526ffb0c602a879c19cd7cd5f45aa32c251

wesm · 2019-02-15T16:04:21Z

Seems there are some correctness issues https://travis-ci.org/apache/arrow/jobs/493616072. @bkietz can you look at the Ruby failure? We should try to reproduce the failure as a C++ unit test

bkietz · 2019-02-15T16:07:36Z

@wesm I'll try to write a unit test which produces the same error

- resize correctly on Finish() - added C++ test

wesm · 2019-02-15T21:06:03Z

cpp/src/arrow/buffer-builder.h

  Status Finish(std::shared_ptr<Buffer>* out, bool shrink_to_fit = true) {
+    // set bytes_builder_.size_ == byte size of data
+    bytes_builder_.UnsafeAdvance(BitUtil::BytesForBits(bit_length_) -
+                                 bytes_builder_.length());


Do these bytes need to be zeroed?

TypedBufferBuilder<bool> doesn't utilize bytes_builder_'s append methods, which means the size of bytes_builder_ doesn't change as bits are appended.

I'm thinking it's not worthwhile to use BufferBuilder in this class, I'm going to refactor and instead just manage a buffer directly

OK, please do that refactoring in a follow up PR and not this one =)

bkietz · 2019-02-15T21:34:32Z

I can't reproduce https://travis-ci.org/apache/arrow/jobs/493915377#L2632 locally

wesm · 2019-02-15T21:39:15Z

OK, I can take it from here then

…endNulls methods

wesm · 2019-02-16T17:23:33Z

Fixed the Ruby test failures and added a C++ unit test that failed.

@xhochy I removed the valid_bytes argument from AppendNulls

https://github.com/apache/arrow/blob/master/cpp/src/arrow/array/builder_primitive.h#L55

If anyone was using this argument, it was not doing anything useful, unless the goal is to append zeroes. I'm not sure if this needs to go through a deprecation cycle. In our codebase it was only being used for internal purposes

wesm · 2019-02-16T23:44:37Z

+1

bkietz changed the title ~~Arrow 4341 primitive builders use bufferbuilder~~ ARROW-4341: [C++] primitive builders use bufferbuilder Feb 6, 2019

wesm reviewed Feb 6, 2019

View reviewed changes

xhochy force-pushed the master branch from 29f76df to 2b9155a Compare February 8, 2019 21:00

xhochy force-pushed the ARROW-4341-primitive-builders-use-bufferbuilder branch from 0cfb57c to 8e2063a Compare February 8, 2019 21:33

bkietz force-pushed the ARROW-4341-primitive-builders-use-bufferbuilder branch from 8e2063a to e8cbe8d Compare February 11, 2019 21:44

bkietz and others added 14 commits February 14, 2019 23:04

Use TypedBufferBuilder<bool> in BooleanBuilder

88eb71c

fix BooleanBuilder::AppendNull

078497a

Use TypedBufferBuilder for PrimitiveBuilder

9c050b4

remove PrimitiveBuilder

88e57fe

add explicit cast

8934573

fix format

188b7b9

moving to iterator append in NumericBuilder

7ba692c

add ArrowLogIgnore and use for release mode DCHECK*

bd736c3

reduce unnecessary zeroing in BufferBuilder

09d2bfe

only zero padding on Finish()

Add TypedBufferBuilder<bool> UnsafeAppend compile-time option to not …

6c1e99d

…track falses. Restore faster code from before this patch for appending C arrays and vector<bool> Change-Id: I01d2d8e87db520a5c38f892a58e86d7a645e8877

Move NumericBuilder implementation to headers to avoid symbol visibil…

d4ab3b5

…ity concerns Change-Id: I5cddcfde0880f5a1af4cc10a3740a186064e85ea

Fix RETURN_NOT_OK usages

21ce285

Change-Id: I749eab94c580775a9faf48b6baa0dabf5dc39111

Restore memory zeroing. Add missing override

3cc5a0c

Change-Id: I6fbf60477bd95d828ff14c54a124c0d079065bfb

Revert change to UnsafeAppend that broke Python unit test

daf5244

Change-Id: Ic32f48c3cd5c5d34494a2e515848e5249c1ca56f

wesm force-pushed the ARROW-4341-primitive-builders-use-bufferbuilder branch from 3d1ea03 to daf5244 Compare February 15, 2019 06:33

Revert changes to arrow/util/logging.h

b389c13

Change-Id: I9f24a526ffb0c602a879c19cd7cd5f45aa32c251

TypedBufferBuilder<bool>'s output was not correctly sized

40c4d8d

- resize correctly on Finish() - added C++ test

wesm reviewed Feb 15, 2019

View reviewed changes

Fix BooleanBuilder::AppendNulls, remove valid_bytes argument from App…

3ef2972

…endNulls methods

wesm changed the title ~~ARROW-4341: [C++] primitive builders use bufferbuilder~~ ARROW-4341: [C++] Refactor Primitive builders and BooleanBuilder to use TypedBufferBuilder<T> Feb 16, 2019

wesm closed this in bbca717 Feb 16, 2019

bkietz deleted the ARROW-4341-primitive-builders-use-bufferbuilder branch February 22, 2019 16:15

asfimport mentioned this pull request Feb 16, 2019

[C++] Use TypedBufferBuilder<bool> in BooleanBuilder #20908

Closed

Uh oh!

ARROW-4341: [C++] Refactor Primitive builders and BooleanBuilder to use TypedBufferBuilder<T> #3575

ARROW-4341: [C++] Refactor Primitive builders and BooleanBuilder to use TypedBufferBuilder<T> #3575

Uh oh!

Conversation

bkietz commented Feb 6, 2019 • edited by wesm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkietz commented Feb 6, 2019

Uh oh!

wesm Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

wesm Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

wesm Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

wesm Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

xhochy commented Feb 8, 2019

Uh oh!

wesm commented Feb 14, 2019

Uh oh!

wesm commented Feb 14, 2019

Uh oh!

wesm commented Feb 14, 2019

Uh oh!

wesm commented Feb 14, 2019

Uh oh!

wesm commented Feb 15, 2019

Uh oh!

wesm commented Feb 15, 2019

Uh oh!

bkietz commented Feb 15, 2019

Uh oh!

wesm Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

bkietz Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

bkietz commented Feb 15, 2019

Uh oh!

wesm commented Feb 15, 2019

Uh oh!

wesm commented Feb 16, 2019

Uh oh!

wesm commented Feb 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bkietz commented Feb 6, 2019 •

edited by wesm

Loading

bkietz Feb 15, 2019 •

edited

Loading