Skip to content

Conversation

@a10y
Copy link
Contributor

@a10y a10y commented Feb 21, 2025

It's very annoying to do random in-place setting of bits in Arrow BooleanBufferBuilder. It's very append focused. For something like SparseArray in particular, you want to create a BoolArray against uninitialized memory and only set the bits corresponding to positions of non-null values.

@gatesn
Copy link
Contributor

gatesn commented Feb 21, 2025

Can we call this BitBuffer?

@a10y a10y marked this pull request as ready for review February 21, 2025 17:10
@a10y a10y marked this pull request as draft February 21, 2025 17:12
Copy link
Contributor

@gatesn gatesn left a comment

Choose a reason for hiding this comment

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

Think we'll also want iterators and vectorized constructors (i.e. BooleanBuffer::collect_bool)

@a10y a10y force-pushed the aduffy/bool-buffer branch 2 times, most recently from 25fc4bd to 67729e2 Compare February 24, 2025 22:51
@a10y a10y marked this pull request as ready for review February 24, 2025 22:52
@a10y
Copy link
Contributor Author

a10y commented Feb 25, 2025

Sadly new_uninit leads to UB when you try and do inplace updates on it. I can't think of a great way to work around that without doing something like having a second bitset, which sort of defeats the point of avoiding the initialization cost anyway, so for now I'm just removing it.

@gatesn
Copy link
Contributor

gatesn commented Mar 1, 2025

Where are we at with this? Need someone to takeover while you poke at FFI bindings?

@a10y
Copy link
Contributor Author

a10y commented Mar 2, 2025

Maybe? I'm no longer sure there's a strong path forward that gives us something better than Arrow's BooleanBufferBuilder.

The big issue is that mapping a bitset on top of uninit memory causes UB when you try and set bits in-place. While it generally seems harmless it is technically a violation of Rust's memory model so not guaranteed to work across platforms.

@gatesn
Copy link
Contributor

gatesn commented Mar 2, 2025

Why does the memory have to be uninitialized? Even the ability to into_mut and update bits is valuable from an API perspective.

@robert3005
Copy link
Contributor

I will take over this pr

@robert3005 robert3005 force-pushed the aduffy/bool-buffer branch from e26e316 to ceff11e Compare May 21, 2025 00:26
@robert3005 robert3005 force-pushed the aduffy/bool-buffer branch 2 times, most recently from 39b9c8a to 7f68ba7 Compare August 14, 2025 13:40
@robert3005 robert3005 added the feature Release label indicating a new feature or request label Aug 14, 2025
@robert3005
Copy link
Contributor

This now compiles but there's still a lot of tests to fix

@codspeed-hq
Copy link

codspeed-hq bot commented Aug 14, 2025

CodSpeed Performance Report

Merging #2456 will degrade performances by 62.42%

Comparing aduffy/bool-buffer (d93320a) with develop (d37444a)

Summary

⚡ 15 improvements
❌ 140 regressions
✅ 1127 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
decompress_rd[f32, 100000] 942.6 µs 854.6 µs +10.29%
decompress_rd[f64, 100000] 1.2 ms 1.3 ms -10.16%
decompress_bitpacking_early_filter[i16, 0.02] 129 µs 149.3 µs -13.55%
decompress_bitpacking_early_filter[i32, 0.02] 138.3 µs 159.2 µs -13.1%
decompress_bitpacking_early_filter[i32, 0.03] 157 µs 187.1 µs -16.09%
decompress_bitpacking_early_filter[i32, 0.04] 174.6 µs 214.2 µs -18.49%
decompress_bitpacking_early_filter[i32, 0.05] 192.5 µs 242.1 µs -20.48%
decompress_bitpacking_early_filter[i64, 0.005] 77.4 µs 86.2 µs -10.12%
decompress_bitpacking_early_filter[i64, 0.02] 158.3 µs 179.9 µs -12.04%
decompress_bitpacking_early_filter[i64, 0.03] 178.8 µs 209.8 µs -14.77%
decompress_bitpacking_early_filter[i64, 0.04] 198.6 µs 239.1 µs -16.93%
decompress_bitpacking_early_filter[i64, 0.05] 218.7 µs 269.2 µs -18.73%
decompress_bitpacking_early_filter[i8, 0.02] 126.2 µs 146.2 µs -13.64%
decompress_bitpacking_early_filter[i8, 0.03] 180.7 µs 209.9 µs -13.9%
decompress_bitpacking_early_filter[i8, 0.04] 193.3 µs 231.9 µs -16.66%
decompress_bitpacking_early_filter[i8, 0.05] 205.7 µs 254.4 µs -19.14%
decompress_bitpacking_late_filter[i16, 0.03] 225.4 µs 254 µs -11.26%
decompress_bitpacking_late_filter[i16, 0.04] 238.6 µs 276.8 µs -13.78%
decompress_bitpacking_late_filter[i16, 0.05] 251.8 µs 299.9 µs -16.04%
decompress_bitpacking_late_filter[i32, 0.0105] 296.4 µs 221 µs +34.13%
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

@robert3005
Copy link
Contributor

I need to figure out all the slow downs - seems like arrows append_packed_range is faster than bitvec implementaion and forcing iterators to be owned adds a bunch of drops that could be avoided

@coveralls
Copy link

coveralls commented Aug 15, 2025

Coverage Status

coverage: 87.875% (+0.2%) from 87.72%
when pulling 2d540e5 on aduffy/bool-buffer
into aaf3e36 on develop.

@codecov
Copy link

codecov bot commented Aug 26, 2025

Codecov Report

❌ Patch coverage is 89.21907% with 156 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.05%. Comparing base (87aa18b) to head (5a48fba).

Files with missing lines Patch % Lines
vortex-buffer/src/bit/buf_mut.rs 82.14% 35 Missing ⚠️
vortex-buffer/src/bit/unaligned.rs 87.43% 26 Missing ⚠️
vortex-buffer/src/bit/buf.rs 89.78% 24 Missing ⚠️
vortex-buffer/src/bit/ops.rs 81.48% 15 Missing ⚠️
encodings/runend/src/compress.rs 61.53% 10 Missing ⚠️
vortex-buffer/src/buffer.rs 73.52% 9 Missing ⚠️
vortex-buffer/src/bit/aligned.rs 88.52% 7 Missing ⚠️
vortex-array/src/arrays/bool/compute/is_sorted.rs 16.66% 5 Missing ⚠️
vortex-array/src/validity.rs 84.61% 4 Missing ⚠️
vortex-array/src/pipeline/canonical.rs 40.00% 3 Missing ⚠️
... and 15 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

gatesn added a commit that referenced this pull request Oct 13, 2025
Pulled from #2456

This separates out the logic of adding the BitBuffer from using it.
There are some outstanding performance issues before we can pick up
#2456 again.

I added some benchmarks that highlight where we're slower than Arrow atm

---------

Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Robert Kruszewski <github@robertk.io>
@robert3005
Copy link
Contributor

Will make a new pr

@robert3005 robert3005 closed this Oct 14, 2025
@robert3005 robert3005 deleted the aduffy/bool-buffer branch October 14, 2025 13:21
gatesn added a commit that referenced this pull request Oct 22, 2025
Replace usages of BooleanBuffer with our own BitBuffer. This lets us
have our own backing Buffer for BitBuffer and implement additional
optimisations for use in vortex.

previously #2456 

This pr requires resolving outstanding performance issues before merging
1. Make iterators cheaper to construct, likely can't be owned
2. Improve append_packed_range performance. Arrow has a function that is
faster than bitvec crate

---------

Signed-off-by: Robert Kruszewski <github@robertk.io>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Co-authored-by: Andrew Duffy <andrew@a10y.dev>
Co-authored-by: Nicholas Gates <nick@nickgates.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Release label indicating a new feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants