BufferIterator can be Send + Sync#8873
Conversation
Signed-off-by: Robert Kruszewski <github@robertk.io>
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
Benchmarks: Vortex queries 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.041x ➖, 0↑ 0↓)
datafusion / parquet (0.970x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.027x ➖, 0↑ 0↓)
duckdb / parquet (0.996x ➖, 0↑ 0↓)
No file size changes detected. |
| end: *const T, | ||
| } | ||
|
|
||
| // SAFETY: `BufferIterator` is a `Buffer<T>` plus two cursors into it, so it can safely be |
There was a problem hiding this comment.
Doesn't the compiler auto-derive Send + Sync if T is so? Right bc of the pointer
|
|
||
| // SAFETY: `BufferIterator` is a `Buffer<T>` plus two cursors into it, so it can safely be | ||
| // `Send`/`Sync` exactly when `Buffer<T>` is. Same bounds as `std::vec::IntoIter`. | ||
| unsafe impl<T: Copy + Send> Send for BufferIterator<T> {} |
There was a problem hiding this comment.
@claude Can't we copy the iterator and share across threads such that it aliases and races?
There was a problem hiding this comment.
Claude finished @0ax1's task in 57s —— View job
Re: "Can't we copy the iterator and share across threads such that it aliases and races?"
Short answer: no, this can't cause a data race. A few reasons stack up:
1. BufferIterator is neither Copy nor Clone.
There's no derive and no manual impl (buffer.rs:709-714), so you literally cannot duplicate it. To move it to another thread you have to move it (Send), which leaves nothing behind on the original thread.
2. Even if it were cloneable, there'd still be no race.
The iterator only ever reads from the buffer — next() does self.ptr.read() (a copy out) and advances its own cursor (buffer.rs:725-733). It never writes into the backing Buffer<T>. Two independent iterators over the same buffer are just two concurrent readers of shared immutable memory, which is exactly what Buffer<T>: Sync already guarantees is safe (same as cloning a slice::Iter or holding two Arc readers).
3. Sync only exposes &self methods, none of which mutate.
Sync means &BufferIterator can be shared across threads. The only thing you can do through a shared & is call size_hint() (buffer.rs:736-740), which reads ptr/end immutably. Advancing the cursor requires next(&mut self), i.e. exclusive access — the borrow checker forbids two threads holding &mut to the same iterator simultaneously. So the aliasing-mutation-race scenario can't be constructed safely.
4. The bounds match the standard library.
T: Send/T: Sync are exactly the bounds std::vec::IntoIter<T> and slice::Iter<'_, T> use. The manual impls are needed only because the two raw pointers (*const T) opt the struct out of the auto-derive (which 0ax1 already noted above); the Send/Sync semantics of the underlying data are unchanged.
So the impls are sound: the cursors are owned per-iterator, the buffer is read-only during iteration, and mutation is gated behind &mut. This is the same reasoning that lets Vec::into_iter() be Send + Sync.
There was a problem hiding this comment.
Claude finished @0ax1's task in 30s —— View job
Yes — this is sound. ✅
I re-read the current code at vortex-buffer/src/buffer.rs:709-740 and the reasoning from my earlier reply holds:
1. The bounds match std exactly.
unsafe impl<T: Copy + Send> Send for BufferIterator<T> {}
unsafe impl<T: Copy + Sync> Sync for BufferIterator<T> {}These are the same bounds std::vec::IntoIter<T> (Send/Sync iff T) and slice::Iter<'_, T> use. The manual unsafe impls are only needed because the two *const T cursors (lines 712-713) opt the struct out of the auto-derive — exactly as you noted. They don't change the Send/Sync semantics of the underlying data.
2. No aliasing is possible.
BufferIterator is neither Copy nor Clone (no derive, no manual impl). To get it onto another thread you must move it (Send), which leaves nothing behind. You cannot end up with two cursors over the same buffer from a single iterator.
3. Iteration is read-only.
next() (lines 725-733) does self.ptr.read() (a copy-out, valid because T: Copy) and advances its own owned cursor. It never writes into the backing Buffer<T>. Even two independent iterators over the same buffer would just be two concurrent readers of shared immutable memory — already guaranteed safe by Buffer<T>: Sync.
4. Sync only exposes non-mutating &self access.
Through a shared &BufferIterator the only reachable method is size_hint() (lines 736-740), which reads ptr/end immutably. Advancing requires next(&mut self), i.e. exclusive access — the borrow checker forbids two threads holding &mut to the same iterator, so the alias-and-race scenario can't be constructed in safe code.
One optional nit (not a soundness issue): size_hint uses unsafe { self.end.offset_from(self.ptr) } without a // SAFETY: comment, while the other unsafe blocks in this struct have one. Adding a short justification (both pointers derive from the same allocation, end >= ptr is an invariant) would keep it consistent with the surrounding style.
So: the impls are sound, and equivalent to why Vec::into_iter() is Send + Sync.
2 participants
BufferIterator is Send and Sync just like Vec IntoIter, fix #8869