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.

Sign up for GitHub

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

Jump to bottom

[Perf] Use SmallVec for some temporary allocations #3

Open
wants to merge 4 commits into
base: mainnet-staging
Choose a base branch
from
Open

[Perf] Use SmallVec for some temporary allocations #3

wants to merge 4 commits into from

Conversation

ljedrz
Copy link
Collaborator

@ljedrz ljedrz commented May 30, 2024

Temporary allocations are ones that are found to be deallocated directly after their creation, meaning they are suitable candidates for a substitution with an array (if the size is known in advance and const) or a SmallVec.

This PR applies the latter in two spots that were found to be a source of a significant number of temporary allocations, and the size was chosen based on values observed empirically in a --dev run.

More SmallVec-related changes will follow, but these 2 require no further setup.

@ljedrz ljedrz changed the title Use SmallVec for some temporary allocations [Perf] Use SmallVec for some temporary allocations May 30, 2024
@ljedrz ljedrz mentioned this pull request May 30, 2024
Open
@ljedrz ljedrz force-pushed the perf/smallvec_in_temp_allocs branch from 16a0ff3 to 8b41f54 Compare May 30, 2024 16:51
@ljedrz
Copy link
Collaborator Author

ljedrz commented May 30, 2024

Added a commit with an extra win.

@vicsn
Copy link
Collaborator

vicsn commented Jun 3, 2024

significant number of temporary allocations

Do you have a rough guess or estimate of the reduction, like you mention in some other SmallVec PRs?

and the size was chosen based on values observed empirically in a --dev run.

If the estimation is wrong, I guess we may see reduced performance compared to if we never had this PR right?

vicsn
vicsn reviewed Jun 3, 2024
View reviewed changes
algorithms/src/crypto_hash/poseidon.rs
@@ -464,7 +464,7 @@ impl<F: PrimeField, const RATE: usize> PoseidonSponge<F, RATE, 1> {
};
let bits = self.get_bits(num_bits_per_nonnative * num_elements);

let mut lookup_table = Vec::<TargetField>::with_capacity(num_bits_per_nonnative);
let mut lookup_table: SmallVec<[TargetField; 256]> = SmallVec::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could it make sense to still use num_bits_per_nonnative?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sadly, it's not const, so we can't use it

@ljedrz
Copy link
Collaborator Author

ljedrz commented Jun 3, 2024 •
edited
Loading

If the estimation is wrong, I guess we may see reduced performance compared to if we never had this PR right?

Some of those sizes (like the number of bits in a Field) will never be breached, and the others were picked with some overhead, so it's unlikely that it would happen often enough to diminish the performance benefits. But in general you are correct - if a SmallVec "spills" often enough, it would be less performant than a Vec.

@ljedrz
Copy link
Collaborator Author

ljedrz commented Jun 3, 2024 •
edited
Loading

Do you have a rough guess or estimate of the reduction, like you mention in some other SmallVec PRs?

These were added incrementally while working on the linked PR, but I'm happy to produce some isolated figures; note that in case of these changes it is more practical to provide them for the specific methods, as the impact on node performance heavily depends on the type of workload.

  • PoseidonSponge::get_fe: own allocs -51%, own temp allocs -99.9%
  • G1::mul_projective: allocs -84%
  • Field::from_bits: allocs -100%

@vicsn vicsn force-pushed the mainnet-staging branch from 74a4378 to d48f6fb Compare June 5, 2024 12:33
@ljedrz ljedrz force-pushed the perf/smallvec_in_temp_allocs branch from 8b41f54 to 4c5211c Compare June 6, 2024 11:12
@vicsn
Copy link
Collaborator

vicsn commented Jan 4, 2025

Mind closing this and rebasing/re-opening on AleoNet? No rush of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vicsn vicsn vicsn left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ljedrz @vicsn