-
Notifications
You must be signed in to change notification settings - Fork 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
feat: poseidon port #180
feat: poseidon port #180
Conversation
Underlying field benchmarks plonky add time: [316.34 ps 319.03 ps 323.61 ps] plonky mul time: [315.96 ps 319.49 ps 326.42 ps] plonky square time: [315.50 ps 316.08 ps 316.72 ps] Rough field op count estimate (for one permutation)
|
Changing x.square() to x * x improves our time from 2 to 1 hash: 1.5236 µs to: 2 to 1 hash: 1.3905 µs |
Hi there, I'm giving this PR a look today and tomorrow. A couple questions:
|
Hi @bgillesp, the port comes from plonky2 https://github.com/0xPolygonZero/plonky2/blob/main/plonky2/src/hash/poseidon.rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid to me! I made sure to compare with the Plonky2 source material fairly carefully, and none of the suggested changes are correctness issues.
pub trait AdaptedField: SmallField { | ||
const ORDER: u64; | ||
|
||
fn from_noncanonical_u96(n_lo: u64, n_hi: u32) -> Self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider changing this function signature to use a tuple input instead of two separate input parameters -- this is the convention used in similar functions in Plonky2 (including reduce_u160
and add_u160_u128
imported just above this), and will make the interfaces more consistent. Something like
fn from_noncanonical_u96((n_lo, n_hi): (u64, u32)) -> Self;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't work, cannot use patterns under trait definition, getting the following error: error[E0642]: patterns aren't allowed in functions without bodies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally right, I guess the syntax here should just be
fn from_noncanonical_u96(n: (u64, u32)) -> Self;
Looks like there are only three references to this declaration so far in poseidon.rs
and poseidon_goldilocks.rs
that would need to be updated if you're okay with change in the function signature.
@bgillesp resolved all comments except for tuple input in the function signature, please have a look again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revisions look good to me! Take a look at the alternate syntax I mentioned for the from_noncanonical_u96
signature in AdaptedField
, but aside from that the PR seems ready to go.
I might also take a quick look now to see if I can hunt down anything that might explain the performance difference between the Plonky2 Poseidon benchmarks and this port.
Initial look at the benchmarks is a bit mystifying: broke down the parts of the permutation bench into full rounds and partial rounds, and both of these are slower in the Ceno port. Further benchmarked the parts of a full round -- the round constant update, the monomial S-box, and the MDS matrix transform -- and strangely, at this level the performance difference vanished. Probably would be worth applying some more in-depth analysis tools to the respective |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing review status to Approve now, though it would be nice to know what is causing the performance regression -- perhaps we can make an issue if it turns out to be difficult to resolve.
@iammadab Can you resolve the merge conflict so that we can merge it ? |
Latest Benchmarks Old Poseidon - 2 to 1 hash: 4.4820 µs - 60 to 1 hash: 23.615 µs Poseidon Port (before changing x.square() to x * x) - 2 to 1 hash: 1.5236 µs - 60 to 1 hash: 12.114 µs Poseidon Port (current) - 2 to 1 hash: 1.3905 µs - 60 to 1 hash: 11.177 µs Plonky2 Poseidon - 2 to 1 hash: 1.0108 µs - 60 to 1 hash: 8.1342 µs
Latest Benchmarks
Old Poseidon
Poseidon Port (before changing x.square() to x * x)
Poseidon Port (current)
Plonky2 Poseidon