Do not hex-format primitives when format doesn't require it #454

Dentosal · 2023-05-22T22:14:13Z

Uses is_human_readable feature of serde. At least postcard which we're currently using for db benefits from this behavior. Works towards FuelLabs/fuel-core#1085

Voxelot · 2023-05-23T01:02:48Z

Should we have unit / snapshot tests to verify that roudtrip serialization works when is_human_readable is enabled / disabled?

Dentosal · 2023-05-23T04:02:16Z

Should we have unit / snapshot tests to verify that roudtrip serialization works when is_human_readable is enabled / disabled?

Yep. Those are already tested for in fuel-core, but it makes sense to have them tested here as well.

xgreenx · 2023-05-23T13:29:42Z

fuel-types/src/array_types.rs

+                    serializer.serialize_str(&format!("{:x}", &self))
+                } else {
+                    let mut seq = serializer.serialize_tuple($s)?;
+                    for elem in self.0 {


The problem here and during deserialization is that you will iterate over each element and serialize/deserialize it separately. It is much slower than copying bytes into/from the slice.

We need to optimize that part for our primitives because we use them everywhere.

Maybe the optimizer already is clever enough to optimize it for arrays, but we need to prove it with benchmarks.

For example, you can reuse implementation form the OptimizedContract to compare with your implementation

The current way is what serde docs suggests to do with fixed size arrays.

You're right that the iteration here is slower than a slice copy; and indeed switching to serialize_bytes speeds this up quite a bit (4x by my microbenchmark). However, with the currently used postcard format, serialize_bytes prefixes the array with it's length in the serialized form, making it larger than necessary. Some other formats (like bincode) do seem not do this, but bincode is much slower otherwise. I wish there was a serde format that had both of these figured out, but I haven't seen that yet.

In any case, I think having a single extra byte encoded for each of the primitives isn't ideal, but we cannot afford the slow down that comes with doing it right, so I'm updating this to use serialize_bytes.

Could you also try to bench it with zerovec, maybe for serialize_bytes, it doesn't use length

Yep, I'll do that later today

So after reading through zerovec docs, it's not apparent to me how zerovec would help us. zerovec is not a serialization format, but rather a set of types to assist with zero-copy deserialization using serde formats. Converting these types to use zerovec would make them less ergonomic to use, and creating new instances would require allocation, as zerovec doesn't do stack-allocated arrays.

What we could do instead is to use Cow<'a, [u8; SIZE]> for the array inside the type instead. This means that the type takes a lifetime argument. Sadly that breaks over 200 different locations where the types are used, and I don't necessarily feel like spending a whole day just adding lifetimes to them in hopes that it would be useful. That would also break things like AsMut trait impl, which is used in many many places as well.

These types are so cheap to copy that zero-copying them makes almost no sense anyways; I think we should instead focus on using zero-copy for the expensive types like Contract.

In the SCALE, they don't put additional bytes for arrays=) Because the size is known

What is SCALE?

Substrate library for serialization and deserialization

Seems to have open correctness issues, but otherwise looks nice.

It seems like the 0-prefixing is outside of our control from the serde impl perspective. We don't really have a way around this since we use postcard downstream, and so avoiding this overhead would require using an entirely different serde adapter (which is outside the scope of this PR imo)

…elLabs/fuel-vm into dento/serde-human-readability

xgreenx

Could you also update the CHANGELOG.md, please?

xgreenx · 2023-05-24T13:58:38Z

fuel-types/Cargo.toml

@@ -19,6 +19,7 @@ serde = { version = "1.0", default-features = false, features = ["derive", "allo
 bincode = { workspace = true }


Hmm, maybe in the other tests we need to use postcard instead of bincode too

Currently we're using both postcard and bincode in our codebase. Tests should use the codec the feature is using, and generally the codec should be an internal implementation detail that's not directly tested.

Hmm, where do we use bincode? I can't find any usage in the fuel-core

Oh apparently we don't anymore. In that case the tests should be updated as well. I'll do a follow-up PR, since that's out-of-scope for this.

Dentosal · 2023-05-24T15:03:50Z

Changelog updated.

Optimize primitive type encoding in non-human-readable serde formats

6bceaad

Dentosal added the fuel-types Related to the `fuel-types` crate. label May 22, 2023

Dentosal self-assigned this May 22, 2023

Clippy

60e3400

Dentosal requested a review from xgreenx May 22, 2023 22:35

Dentosal marked this pull request as ready for review May 22, 2023 22:35

Dentosal requested a review from a team May 22, 2023 22:55

Add roundtrip serialization tests

8186040

Voxelot previously approved these changes May 23, 2023

View reviewed changes

Add comparation of the serialzied and deserialized form

8cfc9cd

xgreenx dismissed Voxelot’s stale review via 8cfc9cd May 23, 2023 13:23

xgreenx reviewed May 23, 2023

View reviewed changes

Dentosal added 6 commits May 23, 2023 19:24

Use bytes-based serialization for speedup

430c456

Merge branch 'dento/serde-human-readability' of https://github.com/Fu…

7b4a4c9

…elLabs/fuel-vm into dento/serde-human-readability

Fix encoded form tests

e18a7f3

fmt

3705179

Merge branch 'master' into dento/serde-human-readability

c32f992

Merge branch 'master' into dento/serde-human-readability

f72f0d0

xgreenx reviewed May 24, 2023

View reviewed changes

Dentosal added 2 commits May 24, 2023 17:57

Merge branch 'master' into dento/serde-human-readability

9708cae

Update changelog

0629b48

Voxelot approved these changes May 24, 2023

View reviewed changes

xgreenx approved these changes May 24, 2023

View reviewed changes

Dentosal added this pull request to the merge queue May 24, 2023

Merged via the queue into master with commit 9b91a19 May 24, 2023

Dentosal deleted the dento/serde-human-readability branch May 24, 2023 16:53

xgreenx mentioned this pull request Jun 12, 2023

Efficient Database Encoding FuelLabs/fuel-core#1085

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not hex-format primitives when format doesn't require it #454

Do not hex-format primitives when format doesn't require it #454

Dentosal commented May 22, 2023 •

edited

Loading

Voxelot commented May 23, 2023

Dentosal commented May 23, 2023

xgreenx May 23, 2023 •

edited

Loading

Dentosal May 23, 2023 •

edited

Loading

xgreenx May 23, 2023

Dentosal May 23, 2023

Dentosal May 23, 2023

xgreenx May 23, 2023

Dentosal May 23, 2023

xgreenx May 24, 2023

Dentosal May 24, 2023

Voxelot May 24, 2023

xgreenx left a comment

xgreenx May 24, 2023

Dentosal May 24, 2023

xgreenx May 24, 2023

Dentosal May 24, 2023

Dentosal commented May 24, 2023

		@@ -19,6 +19,7 @@ serde = { version = "1.0", default-features = false, features = ["derive", "allo
		bincode = { workspace = true }

Do not hex-format primitives when format doesn't require it #454

Do not hex-format primitives when format doesn't require it #454

Conversation

Dentosal commented May 22, 2023 • edited Loading

Voxelot commented May 23, 2023

Dentosal commented May 23, 2023

xgreenx May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Dentosal May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dentosal commented May 24, 2023

Dentosal commented May 22, 2023 •

edited

Loading

xgreenx May 23, 2023 •

edited

Loading

Dentosal May 23, 2023 •

edited

Loading