-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor cpu prover #629
Refactor cpu prover #629
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
encoding/kzg/prover/compute_proof.go
Outdated
m := uint64(len(polyFr)) - 1 | ||
dim := (m - j) / l | ||
|
||
toeV := make([]fr.Element, 2*dimE-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Make 2*dimE-1 a constant
encoding/kzg/prover/compute_proof.go
Outdated
"github.com/consensys/gnark-crypto/ecc/bn254/fr" | ||
) | ||
|
||
type CpuComputer struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Rename to CPUComputer or CPUProofComputer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to CPUProofComputer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think we should add cpu into the name, since in the future, it could have been a gpu. Either cpu or gpu, I don't think it makes sense to have both
encoding/kzg/prover/encode.go
Outdated
"github.com/consensys/gnark-crypto/ecc/bn254/fr" | ||
) | ||
|
||
type ProofComputeDevice interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just ProofComputer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is its old name
Actually, I made a mistake, this optimization would not save us from 8.15 sec to 5.18sec. Probably only going to save us 100-200ms. This is because the bottleneck proving part isn't parallelized, and that total time is 8.15. The parallelization is only saving 100ms for encoding. |
type RsEncodeResult struct { | ||
Frames []rs.Frame | ||
Indices []uint32 | ||
Err error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here and below: it's more readable to move the error to the bottom of this field list
encoding/kzg/prover/compute_proof.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file name could be proof_computer.go
encoding/kzg/prover/compute_proof.go
Outdated
"github.com/consensys/gnark-crypto/ecc/bn254/fr" | ||
) | ||
|
||
type CpuComputer struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to CPUProofComputer
encoding/kzg/prover/encode.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the file name match closer to the content?
If file name is encoder (encode as a verb isn't fit well for file name), maybe the interface below should be ProofEncoder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about proof_device.go
Why are these changes needed?
This PR refactors GPU provers by
The encode task has 5 smaller tasks, and the time spent is the sum of all small task. With the second optimization, the time spent would be the max of all tasks. Empricially speaking, it would reduce the encoding tie from 8.15sec to 5.18 second, which is about 37.5% improvement.
Checks