arena: derive alignment unit from std::max_align_t#2347
arena: derive alignment unit from std::max_align_t#2347benesch wants to merge 1 commit intofacebook:masterfrom
Conversation
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Travis build is failing. Please fix. |
|
Also we got this test failure: https://gist.github.com/yiwu-arbug/c587c19fe82d30e8105758119aea02d5 |
|
@benesch updated the pull request - view changes - changes since last import |
|
Just pushed another build that (should) fix the Travis build failures and the broken arena test. |
|
@benesch updated the pull request - view changes - changes since last import |
|
@benesch updated the pull request - view changes - changes since last import |
|
I had to bump the |
|
@benesch updated the pull request - view changes - changes since last import |
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@siying @IslamAbdelRahman mind take a quick look? I'm not familiar with the arena. |
|
@benesch updated the pull request - view changes - changes since last import |
|
@benesch updated the pull request - view changes - changes since last import |
|
Ok, this is green! @IslamAbdelRahman @siying, any updates? |
c5e7b88 to
e31a17c
Compare
|
@benesch updated the pull request - view changes - changes since last import |
|
@benesch has updated the pull request. View: changes, changes since last import |
facebook-github-bot
left a comment
There was a problem hiding this comment.
@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Trying to see if all the tests pass, before looking at it further. |
e5340d2 to
ce9625c
Compare
|
@benesch has updated the pull request. |
|
Rebased and triggered a new build! |
ce9625c to
3551fe4
Compare
|
@benesch has updated the pull request. |
3551fe4 to
0c37fe3
Compare
|
@benesch has updated the pull request. |
|
Ok, finally fixed the test failure here. The test in question was writing 1000 KV pairs of 100 bytes each, intending to fill up a write buffer of 100,000B and trigger a memtable flush. The new alignment requirements introduced additional overhead that caused the 1000 KVs to spill over into a second memtable where the test was expecting just one. Using KV pairs of 128 bytes and scaling the write buffer accordingly, to 128,000B, avoids the alignment overhead and fixes the test. |
Using `sizeof(void*)` as the alignment unit does not guarantee suitable alignment on all platforms. Darwin, for example, will choke when trying to store a std::function into 8-byte-aligned memory under certain circumstances. Using `alignof(max_align_t)` guarantees that the arena will return suitably-aligned memory. Additionally adjust a column family test to use a key/value size with less alignment overhead (i.e., 128 bytes instead of 100 bytes) to avoid triggering memtable flushes that the test didn't intend.
0c37fe3 to
310f686
Compare
|
@benesch has updated the pull request. |
facebook-github-bot
left a comment
There was a problem hiding this comment.
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@yiwu-arbug is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Great, thanks, @yiwu-arbug! |
As raised in #2265, the arena allocator will return memory that is improperly aligned to store a
std::functionon macOS. Oddly, I'm unable to tickle this bug without adding astd::functionfield tostruct ReadOptions—but my proposal in #2265 does exactly that.In any case, here's a simple reproduction. Apply this bogus patch to get a
std::functionintostruct ReadOptionsthen compile
db_properties_testwith ubsan and runReadLatencyHistogramByLevel:ubsan will complain about several misaligned accesses:
So it seems the root cause is that the internal implementation of
std::functionon macOS (and perhaps with libc++ generally?) requires 16-byte aligned memory, but the arena allocator only guarantees that the returned memory will besizeof(void*)aligned, which is only 8-byte alignment on my machine. This patch solves the problem by adjusting the allocator to derive the necessary alignment fromalignof(std::max_align_t), which is properly 16 bytes on my machine.As I mentioned in #2265, none of RocksDB's tests will cause this unaligned access to actually abort the process, but, on macOS, linking CockroachDB against a version of RocksDB with the above patch and letting it run for just a few seconds will cause a SIGABRT.
I'd get you a backtrace, but Go doesn't include cgo debug information on macOS. I've also tried building against libc++ on Linux, where debug information would be available, but I can't seem to trigger the bug there.
In any case, this PR both fixes the segfault in CockroachDB and fixes the warnings reported by ubsan.