Ensure all of device bitmask is initialized in from_arrow #12668
Conversation
libcudf bitmasks are allocated to a multiple of 64 bytes, in contrast the arrow spec only requires of a column with a null mask that "the validity bitmap must be large enough to have at least 1 bit for each array slot". When the number of rows is not a multiple of 64, the trailing part of the device allocation (which doesn't contribute to actually masking anything) is left uninitialized. While probably benign, this produces errors when running with compute-sanitizer in initcheck mode (since those data are touched and _are_ uninitialized). To fix this, memset the trailing allocation to zero. Closes rapidsai#8873.
wence-
added
libcudf
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## branch-23.04 #12668 +/- ##
===============================================
Coverage ? 85.82%
===============================================
Files ? 158
Lines ? 25184
Branches ? 0
===============================================
Hits ? 21614
Misses ? 3570
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
Nice. For future reference, how did you make it look like I authored a commit on this PR? |
(Although I use magit so it was fewer keystrokes because I got autocompletion of the existing committer names) |
|
FWIW, this is a minimal thing to ensure that loading a bitmask fully initialises things, but isn't enough to make most operations initcheck clean. When we copy a column with a mask from the python side, we end up doing I still need to do some benchmarking of this change before/after to satisfy that it doesn't obviously regress. I'll open up a discussion issue about the broader initcheck story, to see if it is worthwhile to do any more (and if so, how). |
Just to note, I did that in #12668 (comment), and no regression was identified. I think this is still good, and could do with one more C++ review (@bdice or @mythrocks ?) |
|
/merge |
Description
libcudf bitmasks are allocated to a multiple of 64 bytes, in contrast the arrow spec only requires of a column with a null mask that "the validity bitmap must be large enough to have at least 1 bit for each array slot". When the number of rows is not a multiple of 64, the trailing part of the device allocation (which doesn't contribute to actually masking anything) is left uninitialized. While probably benign, this produces errors when running with compute-sanitizer in initcheck mode (since those data are touched and are uninitialized).
To fix this, memset the trailing allocation to zero.
Closes #8873.
Checklist