-
Notifications
You must be signed in to change notification settings - Fork 10
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] Use SmallVec for some temporary allocations #3
base: mainnet-staging
Are you sure you want to change the base?
[Perf] Use SmallVec for some temporary allocations #3
Conversation
16a0ff3
to
8b41f54
Compare
Added a commit with an extra win. |
Do you have a rough guess or estimate of the reduction, like you mention in some other SmallVec PRs?
If the estimation is wrong, I guess we may see reduced performance compared to if we never had this PR right? |
@@ -46,7 +48,7 @@ impl<E: Environment> FromBits for Field<E> { | |||
Ok(Field { field: E::Field::from_bigint(field).ok_or_else(|| anyhow!("Invalid field from bits"))? }) | |||
} else { | |||
// Construct the sanitized list of bits padded with `false` | |||
let mut sanitized_bits = vec![false; size_in_bits]; | |||
let mut sanitized_bits: SmallVec<[bool; 384]> = smallvec![false; size_in_bits]; |
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.
Can you use size_in_bits
?
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.
sadly no, because that one is a trait method (and thus not const
), so it can't be used here; it should be possible in the future
@@ -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(); |
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.
Could it make sense to still use num_bits_per_nonnative
?
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.
sadly, it's not const
, so we can't use it
let to_wnaf = |e: Self::ScalarField| -> Vec<i32> { | ||
let mut naf = vec![]; | ||
let to_wnaf = |e: Self::ScalarField| -> SmallVec<[i32; 128]> { | ||
let mut naf: SmallVec<[i32; 128]> = SmallVec::new(); |
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.
Not sure if worth the effort, but if we plan to merge this PR, I'd prefer to see the 128 be derived from some existing variable. Given that the code is typed on the particular curve to use, preferably we keep the genericity if applicable.
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.
it was picked based on empirical data, but if there is any const
that would be applicable here, I'd be happy to use it
Some of those sizes (like the number of bits in a |
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.
|
Signed-off-by: ljedrz <[email protected]>
Signed-off-by: ljedrz <[email protected]>
Signed-off-by: ljedrz <[email protected]>
Signed-off-by: ljedrz <[email protected]>
8b41f54
to
4c5211c
Compare
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 aSmallVec
.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.