Skip to content

Conversation

@ljedrz
Copy link
Collaborator

@ljedrz ljedrz commented Jun 19, 2025

Isolated from https://github.com/ProvableHQ/snarkVM-fork/pull/3.

Field::from_bits_le is an operation that's omnipresent in heap profiles; by using a SmallVec of the right capacity, we can get rid of all those allocations. The exact scale of the perf improvement depends on the workload.

@ljedrz ljedrz requested a review from vicsn June 19, 2025 09:50
@ljedrz ljedrz marked this pull request as draft June 19, 2025 10:33
@ljedrz ljedrz force-pushed the perf/smallvec_field_frombits branch from a2c25cd to cc95fca Compare June 19, 2025 10:35
@ljedrz ljedrz marked this pull request as ready for review June 19, 2025 15:25
} 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];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use the hardcoded value here? Is it not possible to fetch/use the size_in_bits constant?

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 is impossible, as it's not a constant value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we make Field::size_in_bits a const function, or does the trait need to be dyn compatible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a generic object, so you can't have a Field::<N>::size_in_bits that's const.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about using E::Field::SIZE_IN_BITS? Seems like size_in_bits just calls into that const.

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, that's still generic:

error: generic parameters may not be used in const operations
  --> console/types/field/src/from_bits.rs:52:53
   |
52 | ...SmallVec<[bool; E::Field::SIZE_IN_BITS]> = smallvec![false; size_in_bits];
   |                    ^^^^^^^^^^^^^^^^^^^^^^ cannot perform const operation using `E`
   |
   = note: type parameters may not be used in const expressions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible and a good idea to define a new const and add a unit test somewhere asserting the relevant instantiated values are the same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, as long as we knew what to test in order to find the greatest possible number of bits, we could define a MAX_BITSIZE (currently equal to 384) and check if it's of that size

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.

4 participants