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

Speed up rkyv serialization #123

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jasper-Bekkers
Copy link

This tries to make use of the rkyv CopyOptimize attribute that can guarantee memcpy-able structs are indeed just that, rather then iterating over each element in the vector.

This ran into one oddity however, the archived layout of QbvhNode is smaller then the regular layout (128 vs 124 bytes), this is because QbvhNode contains quite a few padding bytes.

  • QbvhNodeFlags is u8
  • NodeIndex has a u8 member

Both of those cause a (normally) 121-bytes struct to get blown up to 128 bytes.

In this PR I've hackfixed/worked around this issue by making QbvhNodeFlags u64 which obviously isn't what anybody would want, however since I'm not familiar with this project I'd like to open the discussion about what to do with this.

It seems likely that QbvhNode needs to be somehow cache-line aligned (which it is at the moment), but it might be nice to mark the padding bytes in NodeIndex and QbvhNode explicitly so it's clear where in the node one can stuff some additional info.

Removing the CopyOptimize is not the end of the world; but in this case having it speeds up rkyv serialization by about 2x.

@@ -98,7 +98,7 @@ bitflags! {
#[cfg_attr(feature = "cuda", derive(cust_core::DeviceCopy))]
#[derive(Default)]
/// The status of a QBVH node.
pub struct QbvhNodeFlags: u8 {
pub struct QbvhNodeFlags: u64 {
Copy link
Author

Choose a reason for hiding this comment

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

This particular change is the one I wanted to highlight / discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant