Skip to content

Use an array instead of a Vec #3126

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use an array instead of a Vec #3126

wants to merge 3 commits into from

Conversation

richardpringle
Copy link
Contributor

The Vec was always had the same length. There's no reason to use a Vec here, unless I'm missing something

#[serde_as(as = "Vec<Vec<o1_utils::serialization::SerdeAs>>")]
pub mds: Vec<Vec<F>>,
#[serde_as(as = "Vec<[o1_utils::serialization::SerdeAs; 3]>")]
pub mds: Vec<[F; 3]>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What serializer(s) are we using here and what are the backward compatibility requirements? It's possible that we would prefer to serialize as &[F] instead of [F; 3] to maintain backward compatibility (arrays aren't generally serialized with a length). .

Copy link
Member

@dannywillems dannywillems Apr 1, 2025

Choose a reason for hiding this comment

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

Yes, it would probably break compatibility on the serialization level.
However, if the performance is non-negligible, I would be in favor of doing it.

Note that other optimisations can be done at the same time (see below for the contiguous allocated memory or see discussions on Slack to move to Poseidon2 or a Poseidon instance with full/partial rounds), and there is a balance to discuss regarding how/if it would impact the Mina codebase (for instance by requiring a HF).

@@ -9,7 +9,7 @@ use std::str::FromStr;
fn params() -> ArithmeticSpongeParams<Fp> {
ArithmeticSpongeParams {
mds: vec![
vec![
[
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any .sage files that need a similar update?

@dannywillems
Copy link
Member

I like the idea.
I think the initial code was using Vec because the specification of Poseidon does not enforce the size of the state; it is only when you instantiate with specific security parameters that you end up with a fixed value - in our case three.

A further optimisation is to get a single contiguous piece of memory that contains:

  • the matrix coefficients (32 * 3 * 3 bytes)
  • the constant coefficients (3 * 55 * 32 bytes)
  • the sponge state (3 * 32 bytes)
    When doing that in C, like here, I remember I got a ~15% perf improvements. This way, the CPU cache is used as often as possible.

@richardpringle richardpringle self-assigned this Apr 7, 2025
@richardpringle richardpringle force-pushed the array-over-vec branch 2 times, most recently from 9d5d9c0 to 6c33af5 Compare April 8, 2025 16:51
@richardpringle
Copy link
Contributor Author

@dannywillems, there are a lot of round-constants. It's generally not advisable to make stack values that large. I'm not sure I want to make that change (I just needed the inner values of mds to be arrays, I'm not sure I want to make that invasive of a change.

I like the idea. I think the initial code was using Vec because the specification of Poseidon does not enforce the size of the state; it is only when you instantiate with specific security parameters that you end up with a fixed value - in our case three.

A further optimisation is to get a single contiguous piece of memory that contains:

  • the matrix coefficients (32 * 3 * 3 bytes)
  • the constant coefficients (3 * 55 * 32 bytes)
  • the sponge state (3 * 32 bytes)
    When doing that in C, like here, I remember I got a ~15% perf improvements. This way, the CPU cache is used as often as possible.

@richardpringle
Copy link
Contributor Author

It would also probably make sense to lazily initialize these as constants so we can have &'static [[F; 3]] properties. But I think that'll be easier with a newer version of Rust (once those PRs land)

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.

2 participants