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

[Perf] Introduce MulAssign for LinearCombination #1

Draft
wants to merge 3 commits into
base: mainnet-staging
Choose a base branch
from

Conversation

ljedrz
Copy link
Collaborator

@ljedrz ljedrz commented May 24, 2024

Moved from AleoNet#2460; just to recap, this PR improves the performance (both CPU and heap) of Poseidon::hash_many which is prominent in heap profiles, and it consists of 2 changes:

  • introduce MulAssign<&F> for LinearCombination<F>, so that MulAssign<&Field<E>> for Field<E> can avoid going through Mul<&F> for &LinearCombination<F> (which clones the LinearCombination); a relevant benchmark is added too
  • improve memory handling in Poseidon::hash_many via direct tweaks to allocations

These changes reduce the number of allocs/s in a node during a large deployment+execution by ~6% and speeds up the MulAssign<&Field<E>> for Field<E> operation by ~33%:

Field::mul_assign       time:   [43.078 ns 43.184 ns 43.301 ns]
                        change: [-33.164% -32.969% -32.787%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

@@ -463,6 +463,20 @@ impl<F: PrimeField> Mul<&F> for &LinearCombination<F> {
}
}

impl<F: PrimeField> MulAssign<&F> for LinearCombination<F> {
fn mul_assign(&mut self, coefficient: &F) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note: this new implementation is virtually identical to the preexisting Mul<&F> for LinearCombination that is currently being used after first going through Mul<&F> for &LinearCombination

@vicsn
Copy link
Collaborator

vicsn commented May 24, 2024

However cool the find, given everyone's concerns and the limited performance win, I suggest @howardwu to move it back to draft until we have bandwidth and appetite.

@ljedrz
Copy link
Collaborator Author

ljedrz commented May 29, 2024

Oh I can mark it as draft it that's preferable, no problem.

@ljedrz ljedrz marked this pull request as draft May 29, 2024 16:51
@ljedrz ljedrz force-pushed the perf/MulAssign_for_LinearCombination branch from bd08b25 to 729fec2 Compare June 6, 2024 11:18
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