Conversation
886600e to
751034a
Compare
|
I'll look at this a bit, I want to get some other things in before making a breaking change. |
751034a to
f8a1769
Compare
Addressed the things I wanted changed.
There are still a bunch of things I'll be pushing to this PR, but I am currently on leave and will get back to that within next week. |
Ah OK, I will change it to a draft PR until it's ready. Let me know what else you wanted to do when you're back. This PR reminds me that #122 is still an issue for some use cases and perhaps I should revisit options for solving that. |
f8a1769 to
4e19e3c
Compare
|
I've fixed #122 so Vec3A's padding is initialised now which means Vec3A/Mat3A/Affine3A padding is also initialised. Affine2 still contains uninitialised padding. Rebased your PR to latest. |
Every glam type archived to itself, so the archived representation inherited the native type's endianness, alignment and SIMD-backend-dependent layout. Archives were therefore not portable across endianness, nor between glam's own scalar and SIMD backends, and the scalar `Vec3A`/`Mat3A`/`Affine3A` padding was copied into output buffers uninitialised. Give each type an `Archived*` struct built from rkyv's own archived primitives instead. `rkyv::Archived<f32>` already resolves according to rkyv's `unaligned` and `big_endian` features, so one definition covers every configuration without adding a glam feature, and the archived layout no longer depends on which backend glam was compiled with. This also corrects two `unsafe impl`s added in the rkyv 0.8 port: `Portable` claimed a layout identical on all targets, and `NoUndef` claimed an absence of padding that scalar `Vec3A` does not have. Both hold for the new types.
The archived types introduced alongside this are a byte for byte copy of their native form for every type without padding, which lets rkyv serialize slices of them with a single memcpy instead of resolving each element. glam never set COPY_OPTIMIZATION, so this was left on the table before as well. Eligibility is derived from the element type's own hint and a size comparison rather than hardcoded, because it differs per backend: Affine2 has tail padding under SSE2 and NEON but not under scalar-math.
Archived data is what gets uploaded to a GPU or mapped from a file, so the archived types need the same bytemuck impls the native ones have. The derive cannot be used: it would require the element type to be Pod, which rend only is when built with its bytemuck-1 feature, and that is not reachable through rkyv. Also adds From conversions in both directions. Reading a single archived value previously meant either a reference cast, which this branch makes unsound, or rkyv::deserialize with a deserializer and an error type for a conversion that cannot fail.
4e19e3c to
59d96bd
Compare
|
@MarijnS95 did you still have changes you wanted to make in this PR? I've been rebasing with main to keep it up to date but I can stop doing that if it's annoying. It's just the changelog that's getting conflicts. I have another breaking change that I might want to make so I'd try an bundle it with this to minimise churn. Also I added a AI_POLICY.md to glam, as part of that I removed the claude contribution message and it would be good if the PR description was simplified, it's quite verbose and hard to follow for me at the moment. |
Archive glam types to dedicated portable archived types
Every glam type currently archives to itself (
type Archived = Vec3), so the archived representation inherits three properties an archive format should not have:Vec4isalign_of == 16on SSE2/NEON and 4 underscalar-math, which shifts field offsets in any struct embedding one — ascalar-mathbuild cannot read an archive written by a SIMD build of the same version.resolvedoesout.write(*self), so any padding in the native type lands in the archive uninitialised — and every backend has some type with padding, just not the same one. Underscalar-mathit isVec3A/Mat3A/Affine3A, which are#[repr(C, align(16))]over threef32, so four of their sixteen bytes are padding. Under SSE2/NEON those are fine — their fourth lane is real and initialised — butAffine2is 32 bytes there against 24 bytes of fields, becauseMat2becomes 16 byte aligned, leaving 8 bytes of tail padding. Besides being UB, this makes archives non-reproducible, which matters to anyone byte-comparing them.The fix
Give each type an
Archived*struct built from rkyv's own archived primitives, as rkyv's derive already does for user structs:rkyv::Archived<f32>resolves tof32_le/f32_ule/f32_be/f32_ubeper rkyv's ownunalignedandbig_endianfeatures, so one definition covers every configuration and no new glam feature is needed — a crate cannotcfgon another crate's features, so deferring to these aliases is the only mechanism that works.Conversions use
to_array/to_cols_array, which behave identically on every backend, so the archived layout no longer depends on how glam was compiled. Single-byte vectors keep byte elements (Archived<u8>isu8). The archived types are storage only: no#[repr(align(..))], no arithmetic —Deserializerecovers the native type and its alignment.Effects
scalar-math)Vec3AVec4Mat3AAffine3AWith rkyv's
unalignedfeature all of these are align 1, which is the payoff: archives can be read in place from buffers with no alignment guarantee, such as mmap'd database records.This breaks the archived format, and
&ArchivedVec3is no longer a free reference cast to&Vec3— reads convert now. That cast was only valid when the caller controlled both alignment and endianness of the buffer, which a library cannot assume on a consumer's behalf; forVec3A, a 16-aligned SIMD load straight out of an archive was never guaranteed. Happy to put the old behaviour behind an opt-in feature if that is wanted.To keep that from being painful, the archived types carry
Fromconversions in both directions, andbytemuck::Pod/Zeroableunder the existingbytemuckfeature. The latter matters more than it looks: archived data is what gets uploaded to a GPU or written to a file, and whileArchived = Selfthat worked through the native type's ownPodderive. The derive is unusable on the archived types — it would require the element type to bePod, whichrendonly is with itsbytemuck-1feature, not reachable throughrkyv— so these are hand-written impls with the padding and bit-pattern reasoning spelled out. I found both of these by patching this branch into a consumer that streams archived glam buffers straight to the GPU; without them the breakage is much wider than the format change itself.Note on the rkyv 0.8 port
type Archived = $typepredates the 0.8 upgrade, so all three problems are inherited. rkyv 0.7 had no way to state them:resolvewrote through a raw*mut, making the padding copy a latent hazard rather than a claim. The 0.8 port added twounsafe impls that assert the opposite —Portable("layout that is the same on all targets", which the first two points deny) andNoUndef(absence of padding).Place::writeis gated onNoUndefprecisely to keep uninit bytes out of the buffer, so the guard rail was there and the impl asserted past it. glam's bytemuck support already draws this distinction correctly, derivingAnyBitPatternfor scalarVec3AandPodonly for the SIMD ones. Both impls hold unconditionally for the new types.CheckBytesreturningOk(())was sound throughout — theBVecbool vectors are deliberately not covered — but is now delegated to the underlying array so nothing is asserted by hand.That does partly walk back #606, which dropped the validated-access branch from
test_archiveon the grounds that every bit pattern is valid for the primitives glam uses. That reasoning still holds for bit patterns, butrkyv::accessalso checks that the buffer satisfies the archived type's alignment, which is the property this PR is about — so the tests use it again whenbytecheckis enabled, and fall back to unchecked access only when it is not.COPY_OPTIMIZATIONglam never set
Archive::COPY_OPTIMIZATION, so it defaulted todisable()and rkyv serialized aVec<Vec3>element by element. This PR enables it wherever the archived form really is a copy of the native one, which is most types, so the common bulk cases (vertex positions, UVs, transform arrays) become a single memcpy — that also keeps this change from costing consumers throughput on the way past.The condition is derived rather than hardcoded, since eligibility varies by backend:
The first half is rkyv's own
MULTIBYTE_PRIMITIVES_ARE_TRIVIALLY_COPYABLE, false whenever the archive endianness and the target endianness disagree. The second is what rules out padding: the native type holds exactly$nprimitives, so if it is no larger than$nof them there is nowhere for padding to hide. That coversVec2,Vec3,Vec4,Quat,Mat2,Mat3,Mat4andAffine3in both backends, and letsAffine2opt in underscalar-mathwhile turning itself off under SSE2/NEON, with no per-backendcfg.Byte-order identity is the one part not provable from sizes. glam implements
to_array/to_cols_arrayas a transmute for the flat SIMD types and in declaration order for the composites, so it holds — and a test now asserts the archived bytes equal the native bytes for every type where the hint is on, so a future layout change fails the build rather than silently corrupting archives.Open question: the
AtypesVec3A/Mat3A/Affine3Acannot have it, and that is the one real trade-off here: a tight archived form (12 bytes, no uninit) rules the memcpy out forever, while archived-equals-native (16 bytes) keeps it available but copies the padding, whichCopyOptimization::enable()forbids outright. Smaller archives seemed the better default, but happy to go the other way.Giving scalar
Vec3Aan explicit fourth field — as the SIMD backends already have a realwlane — would get both, and let it derivePodinstead ofAnyBitPattern, at the cost of changing the type itself rather than just its rkyv support.Testing
cargo run -p cipasses, as dorkyv,rkyv,bytecheck,rkyv,scalar-math,rkyv,bytecheck,scalar-mathand all numeric-type features. Four new tests: the archived alignment is pinned toArchived<f32>and the archived size to the exact element count, so a futurerepr(align)or padding regression fails the build instead of silently changing the format; the copy optimization is asserted to be on for the types that qualify in both backends and off for the three that never can; andtest_archivenow compares the archived bytes against the native bytes whenever the hint is enabled. I checked those last two are not vacuous by forcing the hint on, which fails onAffine2at 32 against 24 bytes under SSE2. The test helper now uses the checkedrkyv::accesswhenbytecheckis on; the unchecked path remains only for builds without it, where the safety comment now names alignment as well as bit-pattern validity.