Conversation
poly-commit/src/linear_codes/mod.rs
Outdated
| C::Leaf: Default + Clone + Send, | ||
| { | ||
| let ext_mat_cols = ext_mat.cols(); | ||
| let ext_mat_cols = ext_mat.cols().clone(); |
There was a problem hiding this comment.
So we should avoid cloning and rearranging as much as we can. This PR is trying to solve that!
Huh?
There was a problem hiding this comment.
This one is inevitable, unfortunately. H::evaluate consumes the vector as far as I understood. Notice that we were cloning it previously too, but it was not explicit here.
There was a problem hiding this comment.
But with returning a reference, now we just clone t vectors here
https://github.com/HungryCatsStudio/poly-commit/blob/ddfd74d7669e405d15fe5285d1fc0f6e0b5cbd92/poly-commit/src/linear_codes/mod.rs#L599-L602
There was a problem hiding this comment.
In fact, if we mange to derive Copy for Vec<F> we can avoid cloning here too, I guess
There was a problem hiding this comment.
@mmagician Nice!
Also, let me know if I can still help with this PR!
|
This might be helpful now! |
|
@DimitrisPapac could you review this PR please? Maybe we can find a way to keep the cake and eat it too. |
|
Yes, I can get to it later today.
Στις Τρί, 12 Μαρ 2024, 12:59 ο χρήστης Marcin ***@***.***>
έγραψε:
… @DimitrisPapac <https://github.com/DimitrisPapac> could you review this
PR please? Maybe we can find a way to keep the cake and eat it too.
The problem with this change is that we'd be keeping around the entire
matrix *twice*.
(actually, we're also keeping the extended matrix, which is even bigger,
but I don't think we can avoid it due to the commitment_state that's
passed around).
—
Reply to this email directly, view it on GitHub
<#45 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AES2NOWUDHXOW27HOIGEOR3YX3UZZAVCNFSM6AAAAAA7J6R52SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJRGQ4DQMZSG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
The results below are for 12-20 variables and using 10 samples per case. There seems to be a speedup for row-mul branch: brakedown branch: |
|
Looks like a good first step in optimizing the PCS. Btw, |
|
Also @autquis please confirm whether I'm making sense^ if you can |
|
Here are the outputs that I got after benchmarking the row-mul commit: row-mul open: brakedown commit: brakedown open: |
mmagician
left a comment
mmagician
left a comment
There was a problem hiding this comment.
Thanks for the benches!
It seems pretty clear that this is more efficient than brakedown branch.
It also looks better than the previous row-mul, but there the difference is rather small (given num samples). Can you please confirm this with benches against the earlier version of this branch (which only had one Matrix)?
The diff is mostly visible for 20 num vars, we can probably limit the tests to that case, but maybe a little more samples.
| #[cfg(test)] | ||
| pub(crate) fn entry(&self, i: usize, j: usize) -> F { | ||
| self.rows[i][j] |
There was a problem hiding this comment.
Yes, it is not used in Hyrax and in Ligero, it is only for testing. We can remove it.
| pub(crate) fn new_from_rows(row_list: Vec<Vec<F>>) -> Self { | ||
| let m = row_list[0].len(); | ||
| #[cfg(test)] | ||
| pub(crate) fn new_from_rows(row_major: Vec<Vec<F>>) -> Self { |
There was a problem hiding this comment.
Note that RowMajorMatrix is used in Hyrax and Ligero too. We need this method for them. So I would say let's keep it.
|
Hello, all. I ran some more benchmarks, this time comparing performance of the previous commit (hash: e20f67e) and the current commit where we use two different matrices for representation (hash: cf2af29). I increased the number of samples back to 100 and tested for 12, 14, ..., 26 variables. The results can be seen below: Benches for row-mul Benches for row-mul Benches for row-mul Benches for row-mul |
|
What @mmagician said is more efficient than having two representations simultaneously. However, I propose finding better names as we have put it in @DimitrisPapac How come the new technique performs weaker in |
|
Let me throw my tiny two cents in here and say that a way to remove the change information is to remove the previous benchmark output in |
4 participants
So we should avoid cloning and rearranging as much as we can. This PR is trying to solve that!
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pendingsection inCHANGELOG.mdFiles changedin the Github PR explorer