ARROW-2905: [C++] Remove raw_data_ member from BufferBuilder, adaptive int builders#3863
Closed
bkietz wants to merge 7 commits intoapache:masterfrom
Conversation
Member
|
Since ARROW-2905 was opened a good bit of logic is now centralized in buffer-builder.h. There are still raw pointers in use there. Do they impact builder performance? As I recall the prior discussion suggested that the behavior may have changed from gcc 4 to gcc 5+ |
Member
Author
|
Removing DetailsOrthogonal but noteworthy: BooleanBuilder seems to do much better under GCC 4.8 |
Member
|
Thanks for looking further into this! I would like to add a few new benchmarks to be a bit more paranoid. Most of those benchmarks are vector appends and so aren't really going to be affected by this change (compared with scalar append paths) |
Member
|
I can look into that, it's not urgent |
Member
Author
|
Well then should we close this PR until we write scalar append benchmarks? |
Member
|
I can write it today |
Member
|
I can't figure out how to disable Turbo Boost on my Thinkpad P1 so take the benchmarks with a grain of salt, but I don't see an appreciable difference so that's good =) Some of the noise seems to be due to memory allocation before: after: BTW I have started normalizing microbenchmarks to use |
Member
Author
|
Similar reasoning probably applies to PrimitiveArray::raw_values_, ListArray::raw_value_offsets_, BinaryArray::raw_data_, raw_value_offsets_, and UnionArray::raw_type_ids_, raw_value_offsets_ |
wesm
force-pushed
the
ARROW-2905-investigate-if-the-data-pointers-used-in-builder-classes-improve-performance-on-hot-paths
branch
from
f6cdeb8 to
6ace172
Compare
wesm
changed the title
Member
|
Hm looks like there are some valgrind issues |
bkietz
force-pushed
the
ARROW-2905-investigate-if-the-data-pointers-used-in-builder-classes-improve-performance-on-hot-paths
branch
from
6ace172 to
77c78ee
Compare
wesm
force-pushed
the
ARROW-2905-investigate-if-the-data-pointers-used-in-builder-classes-improve-performance-on-hot-paths
branch
from
49c0498 to
d460788
Compare
Member
|
It's hard to really tell whether this is causing an impact or not. The most affected benchmarks by this change are the BinaryBuilder benchmarks, because they append 10-length strings to create an approx 160MB data structure. I increased these benchmarks to run with MinTime 10 seconds, made sure to disable CPU throttling, and I'm showing roughly 1-3% perf degradation before: after This is probably much ado about nothing, but I'm concerned that we lack appropriate benchmarking infrastructure to evaluate this decision. Since there's an effort to establish better benchmarking systems in the next couple of months, I recommend we do not merge this patch now and wait until we can get more data points (using this patch to help determine whether we have a rigorous process in place to determine whether a patch creates a statistically significant performance improvement or regression) |
Codecov Report
@@ Coverage Diff @@
## master #3863 +/- ##
=========================================
+ Coverage 87.76% 88.57% +0.8%
=========================================
Files 727 593 -134
Lines 89507 79926 -9581
Branches 1252 0 -1252
=========================================
- Hits 78557 70791 -7766
+ Misses 10836 9135 -1701
+ Partials 114 0 -114
Continue to review full report at Codecov.
|
Member
|
The |
fsaintjacques
requested changes
Mar 19, 2019
Contributor
fsaintjacques
left a comment
fsaintjacques
left a comment
There was a problem hiding this comment.
There's issues with ignoring AllocateResizableBuffer return status.
bkietz
force-pushed
the
ARROW-2905-investigate-if-the-data-pointers-used-in-builder-classes-improve-performance-on-hot-paths
branch
from
d460788 to
580d284
Compare
yuruiz
reviewed
Apr 23, 2019
| std::shared_ptr<ResizableBuffer> buffer_; | ||
| MemoryPool* pool_; | ||
| uint8_t* data_; | ||
| int64_t capacity_; |
There was a problem hiding this comment.
Just curious, if data_ can be replaced by buffer->data(), then capacity_ can also be replaced by buffer_->capacity() right?
Member
|
This PR seems like a good test case for the benchmark reporter. We also need to look at the performance on Windows since MSVC might be doing different things |
Contributor
|
@bkietz can you rebase, and then use Ursabot to check? |
Member
|
Closing this stale PR, please reopen when it gets picked up again |
bkietz
deleted the
ARROW-2905-investigate-if-the-data-pointers-used-in-builder-classes-improve-performance-on-hot-paths
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removing this member had no appreciable effect on performance in the builder benchmark: