Remove generic_const_exprs feature from EVM crate
#1246
Conversation
| @@ -22,7 +22,16 @@ const QUOTIENT_ORACLE_INDEX: usize = 2; | |||
| /// Represents a STARK system. | |||
| pub trait Stark<F: RichField + Extendable<D>, const D: usize>: Sync { | |||
| /// The total number of columns in the trace. | |||
| const COLUMNS: usize; | |||
| const COLUMNS: usize = Self::EvaluationFrameTarget::COLUMNS; | |||
There was a problem hiding this comment.
Note that keeping this const is not technically needed, but it felt cleaner to use it for the fri methods of the trait, rather than rely on Self::EvaluationFrameTarget::COLUMNS directly there.
There was a problem hiding this comment.
We could also leave it as it was so that it's implementer-specified (with some comment emphasizing that both associated consts must be consistent within Stark, Stark::EvaluationFrame and Stark::EvaluationFrameTarget.
|
I also tested it on ARM targets, both light and beefy machines, and didn't get any noticeable difference in proving time ( |
There was a problem hiding this comment.
This is great; solves the problem with a minimum of fuss! Thanks heaps!
It would be nice to have a solution that avoids all those .try_into().unwrap() type conversions (which is a code smell imho), but I can't think of a way to do that without more extensive structural changes, and those type conversions are pretty harmless here anyway.
| let local_values = | ||
| TryInto::<[P; NUM_CPU_COLUMNS]>::try_into(vars.get_local_values()).unwrap(); | ||
| let local_values: &CpuColumnsView<P> = local_values.borrow(); | ||
| let next_values = | ||
| TryInto::<[P; NUM_CPU_COLUMNS]>::try_into(vars.get_next_values()).unwrap(); | ||
| let next_values: &CpuColumnsView<P> = next_values.borrow(); |
There was a problem hiding this comment.
Nit: I feel like this is a bit more idiomatic (and you have used it in other locations):
| let local_values = | |
| TryInto::<[P; NUM_CPU_COLUMNS]>::try_into(vars.get_local_values()).unwrap(); | |
| let local_values: &CpuColumnsView<P> = local_values.borrow(); | |
| let next_values = | |
| TryInto::<[P; NUM_CPU_COLUMNS]>::try_into(vars.get_next_values()).unwrap(); | |
| let next_values: &CpuColumnsView<P> = next_values.borrow(); | |
| let local_values: &[P; NUM_CPU_COLUMNS] = vars.get_local_values().try_into().unwrap(); | |
| let local_values: &CpuColumnsView<P> = local_values.borrow(); | |
| let next_values: &[P; NUM_CPU_COLUMNS] = vars.get_next_values().try_into().unwrap(); | |
| let next_values: &CpuColumnsView<P> = next_values.borrow(); |
Same comment applies below, and also in keccak_stark.rs.
| @@ -4,7 +4,6 @@ | |||
| #![allow(clippy::type_complexity)] | |||
| #![allow(clippy::field_reassign_with_default)] | |||
| #![feature(let_chains)] | |||
| #![feature(generic_const_exprs)] | |||
|
@unzvfu I'm terribly sorry, I thought my commit addressing your comment had been pushed, will open a follow up PR for cleanup. |
This PR gets rid of `generic_const_exprs` feature entirely, which has wasted us many hours of productivity in the past. As much as it is a cool feature in Rust it is still very much in an unstable state, leading to a lot of `unconstrained generic const` errors, even though our arrays were properly constrained. This PR gets rid of `generic_const_exprs` in the following situations: - within the `Stark` impl entirely. This ties closely with upstream changes to the plonky2 evm - when reviewing, it would be useful to refer to their changes too. It is pretty much the same, except we have an extra public inputs var. See [this PR](0xPolygonZero/plonky2#1246) for their changes. You will notice that I adopted the same stuff basically, especially in our downstream fork of plonky2. - within `HASH_SIZE` of the previously generic hasher, which was also removed upstream previously, [here](0xPolygonZero/plonky2#1024). - in other misc use cases, in `prep_table` for example.
This PR mostly lifts 0xPolygonZero#1246 down into the lib level into the starky crate, getting rid of the unstable `generic_const_exprs` feature from the crate entirely. This should work towards the 0xPolygonZero#417 as well. This would probably warrant a minor version bump in the starky crate to 0.2.0, since this would be an API change for anyone relying on starky.
This PR removes the unstable
generic_const_exprsfeature from the EVM crate. This alleviates both the risk of weird and unpredictable behavior (cf. what happened on #1235 for instance) induced by the high instability of the feature, and the growing compilation time incurred by type-checker getting stuck. This is notably visible when trying to include an eight STARK module, with compilation time completely blowing-up (7min55 vs 1min45 with 7 modules, i.e. +350% increase, in debug mode).The approach somewhat follows the one taken in #1024.
It yields a compilation time improvement of around 20% on my machine, for no noticeable performance downgrade (proving time varies between -3% and +2% across several runs between latest
mainand this branch), although I'd like to extensively ensure this across different test cases and architectures.In summary, this adds two associated types to the
Starktrait, i.e.EvaluationFrameandEvaluationFrameTargetwhich allow to access the local view of a frame (i.e. local and next rows), replacing the previousStarkEvaluationVarsandStarkEvaluationTargetsstructs.Side Note: I haven't dealt with
starkycrate at all, although the same approach could be applied, but I wanted to get validation on the approach before generalizing.