Compute the necessarry inversions of the witness in batch after it's creation #2739
Conversation
| @@ -80,7 +99,7 @@ pub struct Env<Fp, PreImageOracle: PreImageOracleT> { | |||
| pub registers: Registers<u32>, | |||
| pub registers_write_index: Registers<u64>, | |||
| pub scratch_state_idx: usize, | |||
| pub scratch_state: [Fp; SCRATCH_SIZE], | |||
| pub scratch_state: [ToInverseOrNot<Fp>; SCRATCH_SIZE], | |||
There was a problem hiding this comment.
What do you think about using another type of column which cannot be used in further computations, as it is the case for here?
I would approach it with something like allocate_auxiliary_variable (or any better name that can include the idea that it is only to satisfy a single constraint, and should not be used for computation) in the InterpreterEnv interface and add a new column next to ScratchState.
There was a problem hiding this comment.
I do also think, but I need to dig more into the idea, that we can simply commit to the polynomial whose evaluations are not the inverse. I do not know if it will improve the efficiency though. We can discuss it offline.
There was a problem hiding this comment.
There was a problem hiding this comment.
Oopsie. I didn't check before 🙈. I do have a high preference for @querolita solution.
There was a problem hiding this comment.
In general I would say that the same idea can be achieved with a less complex approach. I don't like the verbosity added to represent plain fields vs "to inverse" fields and would prefer to keep it simpler. I wonder why the additional column alias did not seem like a good idea, to me that one had an acceptable amount of "code overhead". I do like the hash map though, if it really compensates the memory consumption (we would need to see examples of real executions). I would feel more inclined to iterate over these two approaches and perform a quick benchmark on the result.
| // The purpose is to perform a batch inversion at the end | ||
| // We symbolically mark the inversion instead of computing them | ||
| #[derive(Clone, Copy)] | ||
| pub enum ToInverseOrNot<Fp> { |
There was a problem hiding this comment.
This is giving me Shakespeare's "to be or not to be" vibes, too verbose for my taste 🫤
There was a problem hiding this comment.
The reason I don't like adding columns, is that I feel that it mixes up concept.
Marking the elements to be inverted seems cleaner to me.
If you do not like the variant type (which I admit is verbose), I can create a set keeping track of the element to be inverted.
What do you think ?
There was a problem hiding this comment.
I think I am biased with the column approach. Maybe it is true that adding a column alias just for to-be-inverted values is a mix of concept. But to me that seemed like a quick way to mark the elements that needed to be inverted. If there's a simpler way to do it without needing an extra column and without that wrapping interface, I would love to look at it. But if it is not straightforward, I wouldn't spend too much time trying to make it look "nice", if a simple enough solution works for now. At least so we can start benchmarking. Perhaps, after doing so, we can come up with alternatives to make it more performant.
There was a problem hiding this comment.
Marking the elements to be inverted seems cleaner to me.
No, it is not cleaner!
There was a problem hiding this comment.
Please, let's keep the version @querolita started.
| pub fn to_field(self) -> Fp { | ||
| match self { | ||
| ToInverseOrNot::NotToInverse(x) => x, | ||
| ToInverseOrNot::ToInverse(x) => { |
There was a problem hiding this comment.
I am not sure this conversion addresses the performance problem we had before. Meaning, this seems not to be batch-inverting all of the elements together, but one after the other, independently of each other.
There was a problem hiding this comment.
It does not indeed, this was to have something working easily.
A later commit removes the to_field function (not in the legacy folder though) and performs the batch inverse
| @@ -368,12 +387,12 @@ impl<Fp: Field, PreImageOracle: PreImageOracleT> InterpreterEnv for Env<Fp, PreI | |||
| self.write_column(pos, res); | |||
| // write the non deterministic advice inv_or_zero | |||
| let pos = self.alloc_scratch(); | |||
| let inv_or_zero = if *x == 0 { | |||
| Fp::zero() | |||
| // write invere or zero | |||
There was a problem hiding this comment.
| // write invere or zero | |
| // write inverse or zero |
| res.evaluations.selector = not_inversed.selector; | ||
|
|
||
| // Collecting the values to inverse | ||
| let to_inverse_map: &mut HashMap<_, F> = &mut HashMap::new(); |
There was a problem hiding this comment.
Okay, I kind of like the idea of having a hash map to avoid repetitions of inverses computations, but at the same time I wonder if the memory overhead is worth the small chance of repeating field elements to be inverted (unless it's very obvious that we always invert the same set of "trivial" values, and by that I mean much smaller sample size than the actual field domain).
There was a problem hiding this comment.
The chance of inverting the same element depends heavily on the use case, so it's hard to say.
Honnestly, I was using a hash map to have a key value mapping more than anything else.
As for the memory overhead, which one are you referring to ?
There was a problem hiding this comment.
I was referring to the actual hash map structure. Not sure how large it can take in memory.
Follow-up of #2047
but we proceed differently.
The scratch state now contains field element which are marked to be inversed or not, using a variant.
The Batch inversion is then done when creating the ProofInput.
We first create an intermediary type NotInversedProofInput, containing the same variant as the scratch state, and implement a conversion to ProofInput which does the batch inversion