[Perf] Use SmallVec for some temporary allocations #3
Conversation
ljedrz
changed the title
ljedrz
force-pushed
the
perf/smallvec_in_temp_allocs
branch
from
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.
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.
Could it make sense to still use num_bits_per_nonnative?
There was a problem hiding this comment.
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.
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.
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 <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
ljedrz
force-pushed
the
perf/smallvec_in_temp_allocs
branch
from
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
--devrun.More
SmallVec-related changes will follow, but these 2 require no further setup.