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

Remove GenericArray #2857

Merged
merged 10 commits into from
Nov 20, 2024
Merged

Conversation

kevinheavey
Copy link

This branches off #2054 so that needs to be merged first

Problem

generic-array is an annoying dependency that is quite avoidable. We can avoid it by hand rolling the serde impls for Signature, or by using the serde_with or serde-big-array crates.

This PR hand rolls the serde impls by closely mimicking what generic-array does (but without the generic stuff). The advantage of this approach is that we should have the same behaviour and performance as before. The disadvantage of this approach is I cargo culted it somewhat so it would be good for someone to look over the unsafe code.

It's possible that serde_with or serde-big-array are better solutions but I haven't investigated the performance or if they differ in any edge cases. I did try serde_bytes and saw that it serialized differently when using short_vec, so the serialization is not trivial.

Summary of Changes

  • Replace GenericArray<u8, U64> with [u8; 64]
  • Hand roll serde impls based on the serde code in generic-array
  • Remove generic-array everywhere else as it is now unused

@kevinheavey kevinheavey force-pushed the signature-no-generic-array branch from 819bfac to bd9cd44 Compare September 6, 2024 16:04
@kevinheavey kevinheavey marked this pull request as ready for review September 6, 2024 17:09
@kevinheavey
Copy link
Author

This is the serde code from the generic-array crate that I've ported over: https://github.com/fizyk20/generic-array/blob/master/src/impl_serde.rs

@kevinheavey kevinheavey force-pushed the signature-no-generic-array branch 2 times, most recently from 28dff09 to 14e9499 Compare September 17, 2024 01:29
@kevinheavey kevinheavey force-pushed the signature-no-generic-array branch 2 times, most recently from 3be8e37 to 26c517c Compare September 24, 2024 17:43
@juchiast
Copy link

juchiast commented Oct 5, 2024

The disadvantage of this approach is I cargo culted it somewhat so it would be good for someone to look over the unsafe code

The unsafe code can be replaced with std::array::try_from_fn, which is in nightly because it use the Residual trait, we could copy std code and make it use Result<T, E> for a safer version.

@kevinheavey kevinheavey force-pushed the signature-no-generic-array branch from 26c517c to 92638a3 Compare November 14, 2024 20:10
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait on this, I wanted to take time to properly test the implementation. Since serialization is simple, I setup a simple bench to see how deserialization performs:

#![feature(test)]
extern crate test;

use {
    solana_signature::Signature,
    std::hint::black_box,
    test::Bencher
};

#[bench]
fn bench(b: &mut Bencher) {
    let sig_bytes: [u8; 64] = [
        01, 02, 03, 04, 05, 06, 07, 08, 09, 10,
        11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
        21, 22, 23, 24, 25, 26, 27, 28, 29, 30,
        31, 32,
        01, 02, 03, 04, 05, 06, 07, 08, 09, 10,
        11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
        21, 22, 23, 24, 25, 26, 27, 28, 29, 30,
        31, 32,
    ];
    let mut reverse_sig_bytes = sig_bytes.clone();
    reverse_sig_bytes.reverse();
    b.iter(|| {
        for _ in 0..100_000 {
            black_box(bincode::deserialize::<Signature>(&sig_bytes).unwrap());
            black_box(bincode::deserialize::<Signature>(&reverse_sig_bytes).unwrap());
        }
    });
}

This PR does exactly the same performance as master:

test bench ... bench:     316,893.35 ns/iter (+/- 665.45)

I also lifted the exact implementation from serde for its arrays, and it got way worse:

test bench ... bench:  27,246,217.40 ns/iter (+/- 765,600.49)

I also tried serde_big_array:

test bench ... bench:     316,795.35 ns/iter (+/- 523.79)

Considering it provides the same performance as all the hand-rolled code, how about we use serde-big-array? I tested build times with this PR and with serde-big-array, and I noticed 0 impact on my machine.

joncinque
joncinque previously approved these changes Nov 20, 2024
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@joncinque joncinque added the automerge automerge Merge this Pull Request automatically once CI passes label Nov 20, 2024
Copy link

mergify bot commented Nov 20, 2024

automerge label removed due to a CI failure

@mergify mergify bot removed the automerge automerge Merge this Pull Request automatically once CI passes label Nov 20, 2024
@joncinque joncinque added the automerge automerge Merge this Pull Request automatically once CI passes label Nov 20, 2024
@mergify mergify bot merged commit ed8ecbb into anza-xyz:master Nov 20, 2024
54 checks passed
@kevinheavey kevinheavey deleted the signature-no-generic-array branch November 20, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge automerge Merge this Pull Request automatically once CI passes need:merge-assist
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants