-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-764: [C++] Improves performance of CopyBitmap and adds benchmarks #1422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
xhochy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, thank you for taking a look at this. There are two code formatting issues reported by Travis. Can you fix these two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you keep this an int64_t?
cpp/src/arrow/util/CMakeLists.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove these reindentations?
e5dd129 to
74e4219
Compare
|
Thanks for taking a look @xhochy. My latest push fixes the formatting issues and also adds a test+fix for a few corner cases my first upload didn't handle correctly. Benchmark with recent changes w.r.t. the comments above in |
|
@seibs Can resolve the two comments in the other pull request? I would then merge both :) |
wesm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, rebased. Will merge once build passes
|
Rebased again to see if build passes... |
|
What are the general performance concerns we are addressing here? I am just trying to understand if there is something similar to be done on JAVA side for bitvector. |
cpp/src/arrow/util/bit-util.cc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should write down explicit types for these autos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced the autos with types.
|
@siddharthteotia there's a couple places in the codebase where we copy bits out of a bitmap, e.g. after slicing vectors arrow/cpp/src/arrow/ipc/writer.cc Line 63 in 2e3832f
|
wesm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Build failing due to some standard flakiness
I took a swing at improving the CopyBitmap performance (benchmarks below). I'm a C/C++ novice, so I thought I'd get some feedback before I went too much further.
Starting Point
Using stanford bithacks for SetBitTo
memcpy + shifting
This solution provides varying performance depending on whether or not the bit offset is a multiple of 8