Skip to content

ARROW-1290: [C++] Double buffer size when exceeding capacity in arrow::BufferBuilder as in array builders#905

Closed
wesm wants to merge 1 commit intoapache:masterfrom
wesm:ARROW-1290
Closed

ARROW-1290: [C++] Double buffer size when exceeding capacity in arrow::BufferBuilder as in array builders#905
wesm wants to merge 1 commit intoapache:masterfrom
wesm:ARROW-1290

Conversation

@wesm
Copy link
Member

@wesm wesm commented Jul 28, 2017 •
edited
Loading

Kind of an embarrassing oversight, but it's good that we caught it.

In a test for incrementally building a BinaryArray, this yields about 4x speedup

Benchmark                                     Time           CPU Iterations
---------------------------------------------------------------------------
BM_BuildBinaryArray/repeats:3             11892 us      11892 us         59   840.886MB/s
BM_BuildBinaryArray/repeats:3             11903 us      11904 us         59   840.082MB/s
BM_BuildBinaryArray/repeats:3             11909 us      11910 us         59   839.662MB/s
BM_BuildBinaryArray/repeats:3_mean        11902 us      11902 us         59    840.21MB/s
BM_BuildBinaryArray/repeats:3_stddev          7 us          7 us          0   520.137kB/s

before:

Benchmark                                     Time           CPU Iterations
---------------------------------------------------------------------------
BM_BuildBinaryArray/repeats:3             45678 us      45571 us         15   219.439MB/s
BM_BuildBinaryArray/repeats:3             45416 us      45209 us         15   221.197MB/s
BM_BuildBinaryArray/repeats:3             45227 us      45122 us         15   221.619MB/s
BM_BuildBinaryArray/repeats:3_mean        45440 us      45301 us         15   220.752MB/s
BM_BuildBinaryArray/repeats:3_stddev        185 us        194 us          0   966.716kB/s

@wesm
Double buffer size when exceeding capacity in arrow::BufferBuilder, l…
59d4d9c 
…ike in other array builder classes

Change-Id: Ibb9328dc9482ce334c4f434f7479079414f5e301
@jeffknupp
Copy link
Contributor

jeffknupp commented Jul 28, 2017

@wesm yes, I'll test both with and without jemalloc

@jeffknupp
Copy link
Contributor

jeffknupp commented Jul 28, 2017

Certainly fixed in the non-jemalloc case. Going to test with jemalloc 4.5.0 now.

@jeffknupp
Copy link
Contributor

jeffknupp commented Jul 28, 2017

The jemalloc issue remains (which makes sense, since it's happening in a single threaded process and all this change does is reduce the frequency of reallocations)

xhochy
xhochy approved these changes Jul 28, 2017
View reviewed changes
Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM

@asfgit asfgit closed this in 4df2a0b Jul 28, 2017
@wesm wesm deleted the ARROW-1290 branch July 29, 2017 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xhochy xhochy xhochy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wesm @jeffknupp @xhochy