(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, …?
|
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 |
mcalancea
force-pushed
the
cleanup/safe-row-major-matrix-variant
branch
from
171ca51
to
7b11a81
Compare
### Context Existing benchmark `riscv_add` are just for testing single opcode performance. This PR aim to have a first e2e bench which is representative to a workload in real world. ### What has been cover - add fibonacci bench with sp1 platform hardcode - refactor e2e flow into sub-function to maximize code sharing ### what's not being covered Hey @mcalancea, key generation + witness assignment are still encaptulated in same function and not including in bench latency meature. Break down key generation and extract witness assignment will expose massive intermediate variables in between, so probably need another refactor and design to support #624
No description provided.