(WIP) Refactor row major matrix #624
Conversation
|
This is a great effort! As some moment I am also curious what would be the impact if we remove MaybeUint feature and use raw vector as we only have one implementation. I just do a very quick tried on this branch against master branch on riscv_add benchmark with command on remote ceno benchmark machine, with command and the result turns out to be ~+6% slower on e2e latency. I believed with fibonacchi example it might be even much slower. So it's still nessesary to this feature for keeping high performance. |
|
If we just talked about capture this unssignment error, I think we can achieve it from different path: in unittest we support init vector into 2 different default value with same execution trance, and then compare 2 witnesses which should be identical. An unequality indicate there are some unassigment bug. And with that, we can capture the unassignment error in early stage. |
|
Thank you! This is still work in progress; I'm examining some ways to make it faster while keeping it clean/safe. My bench results are a bit better for some reason: What regression % (evaluated at yours) would you consider acceptable? |
| pub fn num_instances(&self) -> usize { | ||
| self.values.len() / self.num_col - self.num_padding_rows | ||
| pub fn len(&self) -> usize { | ||
| self.values.len() |
There was a problem hiding this comment.
Confusing name. Maybe total_len, num_cells, …?
| let start = Instant::now(); | ||
| let padding_row = match self.padding_strategy { | ||
| InstancePaddingStrategy::RepeatLast => { | ||
| self.values[self.values.len() - self.num_col..].to_vec() |
There was a problem hiding this comment.
What if empty?
There was a special case if steps.is_empty() somewhere:
InstancePaddingStrategy::RepeatLast if steps.is_empty() => {
tracing::debug!("No {} steps to repeat, using zero padding", Self::name());
vec![MaybeUninit::new(E::BaseField::ZERO); num_witin]
}although that was not completely correct either; better than crashing.
|
Could you please add to the PR description a very brief overview over what we are doing, and more importantly why? (The what can be really brief, because the text of the PR answers that question in detail.) Thanks! |
Yes, I have been rather suspicious of |
I noticed the different with yours vs remote ceno benchmark server probably on environment number of cores. To me any regression of % is somehow unacceptable. So I would suggest to go from another way: in unittest initialized with 2 different default value with same witness, then check the witness polynomial equality. This help us to identify the problem while not sacrificing performance. |
|
If you want some inspiration for how to do this kind of change, you might want to have a look at how plonky3 changed the organisation of its data compared to plonky2. (It might be a bit much too read, though.) |
|
@hero78119 There is a draft of the approach based on testing here: #597 |
|
To get back to optimal performance without unsafe types, there could be another constructor that accepts a vector or an iterator. Callers can built their Vec with e.g. flatten and collect. See also the issue #600 |
No description provided.