Add an ArrayBuffer that declares alignment#1720
Conversation
| // TODO(ngates): enforce 128 byte alignment once we have a ScalarBufferBuilder that can | ||
| // enforce custom alignments. | ||
| // let packed = ArrayBuffer::new_with_alignment(packed, FASTLANES_ALIGNMENT); | ||
| let packed = ArrayBuffer::new_with_alignment(packed, ptype.byte_width()); |
There was a problem hiding this comment.
new_with_alignment (and new) are weird names for a thing called ArrayBuffer, since I kind of expect them to allocate? a la Vec::new
| length: buffer.len() as u64, | ||
| padding, | ||
| // Buffer messages have no minimum alignment, the reader decides. | ||
| alignment_: 0, |
There was a problem hiding this comment.
0 is not a power of 2, will this cause a panic?
There was a problem hiding this comment.
This is flatbuffer default value. Reader must handle it
lwwmanning
left a comment
lwwmanning
left a comment
There was a problem hiding this comment.
I wonder if AlignedBuffer would be a better name for ArrayBuffer? and should it be in the vortex-buffer crate? (or like, should Buffer even be pub since we should presumably ~always be using this?)
| .vortex_expect("Invalid validity"), | ||
| }), | ||
| Some(buffer), | ||
| Some(ArrayBuffer::new_with_alignment(buffer, ptype.byte_width())), |
There was a problem hiding this comment.
should constructor just accept ArrayBuffer directly?
There was a problem hiding this comment.
It should accept a ScalarBuffer, and then the PType can be extracted. For now it's a bit annoying to take an ArrayBuffer and then make sure alignment matches when I'm about to change it anyway
| packed.len() | ||
| )); | ||
| } | ||
| let packed = ArrayBuffer::new_with_alignment(packed, FASTLANES_ALIGNMENT); |
There was a problem hiding this comment.
For now, I expect this will fail. I can fix this up in the PR to add ScalarBuffer by specifying a runtime alignment.
| length: buffer.len() as u64, | ||
| padding, | ||
| // Buffer messages have no minimum alignment, the reader decides. | ||
| alignment_: 0, |
There was a problem hiding this comment.
This is flatbuffer default value. Reader must handle it
| .vortex_expect("Invalid validity"), | ||
| }), | ||
| Some(buffer), | ||
| Some(ArrayBuffer::new_with_alignment(buffer, ptype.byte_width())), |
There was a problem hiding this comment.
It should accept a ScalarBuffer, and then the PType can be extracted. For now it's a bit annoying to take an ArrayBuffer and then make sure alignment matches when I'm about to change it anyway
| self.buffer | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
might be nice to offer a way to get a slice of T out of this where we check the alignment of T at runtime
|
Closed for #1742 |
3 participants
This is so that the reader knows the minimum alignment required for a given array buffer.
This can help minimize copies during the read when buffers are already sufficiently aligned.
It also allows e.g. FastLanes to declare much wider alignment, for example 128 bytes.