Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the Arrow Mesh3D type to use zero-copy Buffers #1691

Merged
merged 2 commits into from
Mar 23, 2023

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Mar 23, 2023

For the arkit scene example this reduces visit time from 7ms to 20us.

Before:
image

After:
image

Checklist

@jleibs jleibs marked this pull request as ready for review March 23, 2023 16:42
@jleibs jleibs added the 📉 performance Optimization, memory use, etc label Mar 23, 2023
@Wumpf Wumpf self-requested a review March 23, 2023 16:52
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

nice!!

@@ -121,7 +119,7 @@ impl LoadedMesh {
let vertex_colors = if let Some(vertex_colors) = vertex_colors {
vertex_colors
.iter()
.map(|c| Rgba32Unmul::from_rgba_unmul_array(c.to_array()))
.map(|c| Rgba32Unmul::from_rgba_unmul_array(ColorRGBA(*c).to_array()))
Copy link
Member

Choose a reason for hiding this comment

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

c is not already an array/slice that can be passed directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we're basically doing an endianess swap here.

ColorRGBA stores the color as 0xRRGGBBAA, but to_array converts it to [r,g,b,a].

    pub fn to_array(&self) -> [u8; 4] {
        [
            (self.0 >> 24) as u8,
            (self.0 >> 16) as u8,
            (self.0 >> 8) as u8,
            self.0 as u8,
        ]
    }

Copy link
Member

Choose a reason for hiding this comment

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

it's a matter of turning a u32 into a Rgba32Unmul. We could add a Rgba32Unmul::from_u32 helper but then we would need to document exactly how that works etc…

@emilk
Copy link
Member

emilk commented Mar 23, 2023

350x speedup - not bad

not-bad

@jleibs jleibs merged commit 6070aa8 into main Mar 23, 2023
@jleibs jleibs deleted the jleibs/zero_copy_mesh branch March 23, 2023 17:35

/// Per-vertex albedo colors.
pub vertex_colors: Option<Vec<ColorRGBA>>,
/// This is actually an encoded [`super::ColorRGBA`]
pub vertex_colors: Option<Buffer<u32>>,
Copy link
Member

Choose a reason for hiding this comment

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

Hum, why doesn't Buffer<ColorRGBA> work?

Copy link
Member Author

@jleibs jleibs Mar 23, 2023

Choose a reason for hiding this comment

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

In short because deserialization support for buffer is implemented as:

// Blanket implementation for Buffer
impl<T> ArrowDeserialize for Buffer<T>
where
    T: ArrowDeserialize + NativeType + ArrowEnableVecForType,
    for<'b> &'b <T as ArrowDeserialize>::ArrayType: IntoIterator,
{
    type ArrayType = ListArray<i32>;

    #[inline]
    fn arrow_deserialize(
        v: <&Self::ArrayType as IntoIterator>::Item,
    ) -> Option<<Self as ArrowField>::Type> {
        v.map(|t| {
            t.as_any()
                .downcast_ref::<PrimitiveArray<T>>()
                .unwrap()
                .values()
                .clone()
        })
    }
}

Similar to our workaround for BinaryBuffer, we could probably do something like implement a struct ColorRGBABuffer(Buffer<ColorRGBA>), to specialize it.

I was hoping I could just make ColorRGBA act as a native type, but arrow2::NativeType is sealed: https://github.com/jorgecarleitao/arrow2/blob/main/src/types/native.rs

@@ -190,7 +190,7 @@ impl RawMesh3D {
return Err(RawMeshError::IndicesNotDivisibleBy3(indices.len()));
}

for &index in indices {
for &index in indices.iter() {
Copy link
Member

Choose a reason for hiding this comment

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

we should impl IntoIter for &BinaryBuffer… but whatever

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📉 performance Optimization, memory use, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants