Skip to content

Upgrade to rkyv 0.8 #1502

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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Upgrade to rkyv 0.8 #1502

wants to merge 8 commits into from

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented Mar 28, 2025

  • ⚠️ To be noted that data serialized by rkyv 0.8 is not compatible with data serialized with rkyv 0.7.

Helpful resources

helpful commits from rkyv

Current state

Difficulties

  • I did not succeed in keeping rkyv(as = ) for Matrix.
    • I thought that was because i32 is not Portable. See WIP migration to 0.8 rkyv/rkyv_contrib#4 for a somewhat similar situation.
    • But I was able to implement it for ArrayStorage, so it must be something else in play.
    • I believe using as = makes it impossible to use rkyv(derive(...)), so we have no other choice to implement those manually. It's a bit boiler-plate-y because of _le rkyv types.
Previous analysis for `as`

In current nalgebra, we're using:

#[repr(transparent)]
#[derive(Copy, Clone, PartialEq, Eq, Hash)]
#[derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize, bytecheck::CheckBytes)]
#[archive(
    as = "ArrayStorage<T::Archived, R, C>",
    bound(archive = "
        T: rkyv::Archive,
        [[T; R]; C]: rkyv::Archive<Archived = [[T::Archived; R]; C]>
    ")
)]
pub struct ArrayStorage<T, const R: usize, const C: usize>(pub [[T; R]; C]);

I believe we'll want to keep using that as = ; I'm not sure what's the advantage of that exactly, I guess it limits the number of types, to avoid ending up with too many archived types, as well as avoid type juggling. (so compilation and runtime performance) ; ❓ any guess confirmation is welcome 😄

I ended up with this migration, mostly reading (and forcing) through errors:

#[repr(transparent)]
#[derive(rkyv::Archive, rkyv::Portable, Serialize, Deserialize, bytecheck::CheckBytes)]
#[rkyv(
        as = ArrayStorage<T::Archived, R, C>,
        archive_bounds(
            T: rkyv::Archive,
            [[T; R]; C]: rkyv::Archive<Archived = [[T::Archived; R]; C]>
        )
    )
]
pub struct ArrayStorage<T, const R: usize, const C: usize>(pub [[T; R]; C]);

  • Implementation of PartialEq between arraystorage is currently not generic

Vrixyz added a commit to Vrixyz/rkyv-playground that referenced this pull request Apr 1, 2025

let archived = rkyv::check_archived_root::<$ty<f32>>(&bytes[..]).unwrap();
let archived = rkyv::access::<<$ty<f32> as rkyv::Archive>::Archived, rkyv::rancor::Error>(&bytes)
.unwrap();
// Compare archived and non-archived
assert_eq!(archived, &value);

Copy link
Contributor Author

@Vrixyz Vrixyz Apr 2, 2025

Choose a reason for hiding this comment

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

The debug check underneath is adding a constraint on Archived to be Debug, which was not explicit before, I think that's what we'll want to do like what I did for Matrix through archive_bounds(<S as Archive>::Archived: core::fmt::Debug), but it feels like it's adding potentially unnecessary constraints.

Could a manual implementation could implement Debug only for compatible types ?

Also, the debug format test is failing currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

going through ~as = Matrix<Archived::T> and leveraging Portable is probably the best path forward, but I'm not sure how exactly.

compare(PartialEq, PartialOrd),
derive(Debug),
archive_bounds(
<S as Archive>::Archived: core::fmt::Debug,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like this is a bit constraining ? This is only needed for automated derive (needed for tests)

See https://github.com/dimforge/nalgebra/pull/1502/files#r2025182401

@@ -161,16 +157,18 @@ pub type MatrixCross<T, R1, C1, R2, C2> =
#[cfg_attr(
feature = "rkyv-serialize-no-std",
derive(Archive, rkyv::Serialize, rkyv::Deserialize),
Copy link
Contributor Author

@Vrixyz Vrixyz Apr 2, 2025

Choose a reason for hiding this comment

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

Do we want to derive rkyv::Portable ? I'm not sure what the implications are.

I think this would allow us to avoid a specific ArchivedMatrix being created, which is likely what we want, as was the case with as Matrix.

/// A translation.
#[repr(C)]
#[cfg_attr(
feature = "rkyv-serialize-no-std",
derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize),
archive(
as = "Translation<T::Archived, D>",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This as being removed is a UX downgrade because we can't access underlying functions from original type on rkyv archived type.

@@ -5,21 +5,21 @@ use na::{
Rotation3, Similarity3, SimilarityMatrix2, SimilarityMatrix3, Translation2, Translation3,
};
use rand;
use rkyv::{Deserialize, Infallible};

macro_rules! test_rkyv_same_type(
($($test: ident, $ty: ident);* $(;)*) => {$(
#[test]
fn $test() {
let value: $ty<f32> = rand::random();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should add a new test on i32 or something different than f32, as currently this would fail due to missing partialEq from arraystorage.

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