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
| @@ -128,6 +129,8 @@ impl MessageEncoder { | |||
| &fba::BufferArgs { | |||
| 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
| @@ -60,7 +61,7 @@ impl PrimitiveArray { | |||
| .to_metadata(length) | |||
| .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
| @@ -87,6 +90,7 @@ impl BitPackedArray { | |||
| 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.
| @@ -128,6 +129,8 @@ impl MessageEncoder { | |||
| &fba::BufferArgs { | |||
| 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
| @@ -60,7 +61,7 @@ impl PrimitiveArray { | |||
| .to_metadata(length) | |||
| .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
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.