Skip to content

Improve byte array serialization #12

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

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

robin-nitrokey
Copy link
Member

@robin-nitrokey robin-nitrokey commented Feb 22, 2023

Per default, byte arrays are serialized inefficiently by serde.

serde_bytes provides workarounds for byte slice serialization but does not support arrays yet. heapless_bytes improves the serialization for arrays but does not guarantee their length. Therefore we introduce a helper type, BytesArray, that uses heapless_bytes’ serialization but enforces a constant length.

serde-byte-array provides a wrapper type with a more efficient serialization format.

Fixes #11

@sosthene-nitrokey
Copy link
Contributor

It's a bit of a pain that dtolnay completely ignores my pr to serde_bytes.

Maybe we should publish a fork of serde-bytes as serde-bytes-array? We already needed something like this in opcard (though we don't anymore).

src/util.rs Outdated
/// the constant size `N`.
#[derive(Debug, Deserialize, Serialize)]
#[serde(transparent)]
pub struct BytesArray<const N: usize>(Bytes<N>);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather take the approach of wrapping a [u8;N] and manually implementing Serialize, Deserialize and Deref on them (which I think also gives us the ConstantTimeEq implementation. This avoids the unnecessary overhead of the Vec and the runtime checks.

Maybe just take my existing implementation?

Copy link
Member Author

@robin-nitrokey robin-nitrokey Feb 22, 2023

Choose a reason for hiding this comment

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

That was my first approach, but I’d rather not manually implement Deserialize here. That just duplicates code from serde-bytes or heapless-bytes and requires more testing than just wrapping Bytes.

@robin-nitrokey
Copy link
Member Author

Maybe we should publish a fork of serde-bytes as serde-bytes-array? We already needed something like this in opcard (though we don't anymore).

Does this have to be a fork? Can’t we have a serde-byte(s)-array crate that just provides the Byte(s)Array type and, if needed, depends on serde-bytes?

@sosthene-nitrokey
Copy link
Contributor

It doesn't necessarily have to be a fork no. We could have just the [u8;N] support in its own crate.
Maybe it could be part of heapless-bytes?

@robin-nitrokey
Copy link
Member Author

I’d prefer a separate crate. Mabye other people are also interested in using this specialization without pulling in heapless.

@sosthene-nitrokey
Copy link
Contributor

I pushed one at https://github.com/Nitrokey/serde-byte-array

@robin-nitrokey
Copy link
Member Author

I pushed one at https://github.com/Nitrokey/serde-byte-array

404 – is it a private repository?

@sosthene-nitrokey
Copy link
Contributor

I made it public
Should we publish it on crates.io?

@robin-nitrokey
Copy link
Member Author

Thanks! I’ll make a PR with some minor changes and then we can release.

Per default, byte arrays are serialized inefficiently by serde.
serde-byte-array provides a wrapper type with a more efficient
serialization format.

Fixes trussed-dev#11
@robin-nitrokey robin-nitrokey merged commit 6b9e316 into trussed-dev:main Feb 24, 2023
@robin-nitrokey robin-nitrokey deleted the serde-bytes branch February 24, 2023 10:23
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.

Use efficient byte representation
2 participants