-
Notifications
You must be signed in to change notification settings - Fork 198
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
optimize encoder interpolation latency #309
Changes from 12 commits
50a52b2
88ffc29
97b5e39
d8b637b
09e4e19
c6ba77b
575c1df
84dc3c0
55c9882
2ee68eb
331b14d
c981800
12c2526
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package rs | |
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"log" | ||
"time" | ||
|
||
|
@@ -56,8 +57,8 @@ func (g *Encoder) Encode(inputFr []fr.Element) (*GlobalPoly, []Frame, []uint32, | |
return nil, nil, nil, err | ||
} | ||
|
||
log.Printf(" SUMMARY: Encode %v byte among %v numNode takes %v\n", | ||
len(inputFr)*encoding.BYTES_PER_COEFFICIENT, g.NumChunks, time.Since(start)) | ||
log.Printf(" SUMMARY: RSEncode %v byte among %v numChunks with chunkLength %v takes %v\n", | ||
len(inputFr)*encoding.BYTES_PER_COEFFICIENT, g.NumChunks, g.ChunkLength, time.Since(start)) | ||
|
||
return poly, frames, indices, nil | ||
} | ||
|
@@ -72,34 +73,51 @@ func (g *Encoder) MakeFrames( | |
if err != nil { | ||
return nil, nil, err | ||
} | ||
k := uint64(0) | ||
|
||
|
||
indices := make([]uint32, 0) | ||
frames := make([]Frame, g.NumChunks) | ||
|
||
for i := uint64(0); i < uint64(g.NumChunks); i++ { | ||
numWorker := uint64(g.NumRSWorker) | ||
|
||
// finds out which coset leader i-th node is having | ||
j := rb.ReverseBitsLimited(uint32(g.NumChunks), uint32(i)) | ||
if numWorker > g.NumChunks { | ||
numWorker = g.NumChunks | ||
} | ||
|
||
// mutltiprover return proof in butterfly order | ||
frame := Frame{} | ||
indices = append(indices, j) | ||
jobChan := make(chan JobRequest, numWorker) | ||
results := make(chan error, numWorker) | ||
|
||
ys := polyEvals[g.ChunkLength*i : g.ChunkLength*(i+1)] | ||
err := rb.ReverseBitOrderFr(ys) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
coeffs, err := g.GetInterpolationPolyCoeff(ys, uint32(j)) | ||
if err != nil { | ||
return nil, nil, err | ||
for w := uint64(0); w < numWorker; w++ { | ||
go g.interpolyWorker( | ||
polyEvals, | ||
jobChan, | ||
results, | ||
frames, | ||
) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: move the |
||
k := uint64(0) | ||
for i := uint64(0); i < g.NumChunks; i++ { | ||
j := rb.ReverseBitsLimited(uint32(g.NumChunks), uint32(i)) | ||
jr := JobRequest{ | ||
Index: uint64(i), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
FrameIndex: k, | ||
} | ||
jobChan <- jr | ||
k++ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is |
||
indices = append(indices, j) | ||
} | ||
close(jobChan) | ||
|
||
frame.Coeffs = coeffs | ||
for w := uint64(0); w < numWorker; w++ { | ||
interPolyErr := <-results | ||
if interPolyErr != nil { | ||
err = interPolyErr | ||
} | ||
} | ||
|
||
frames[k] = frame | ||
k++ | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("proof worker error: %v", err) | ||
} | ||
|
||
return frames, indices, nil | ||
|
@@ -127,3 +145,38 @@ func (g *Encoder) ExtendPolyEval(coeffs []fr.Element) ([]fr.Element, []fr.Elemen | |
|
||
return evals, pdCoeffs, nil | ||
} | ||
|
||
type JobRequest struct { | ||
Index uint64 | ||
FrameIndex uint64 | ||
} | ||
|
||
func (g *Encoder) interpolyWorker( | ||
polyEvals []fr.Element, | ||
jobChan <-chan JobRequest, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can it just use a workerpool, instead of a dedicated channel to create job request There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is actually a good idea, I think we should create a global persistent object under prover? what do you think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why global/persistent? Is the concern in workpool creation/destroy cost? It's generally better not have global/persistent object if can be avoided. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Say each requests is 256KB, 10MBps is equivalent to 40 parallel requests, and they need to share threads. If multiple objects are specified, it is unclear how to allocate them. I will mark it as todo, since I don't know the performance of the threadpool, compared to now. |
||
results chan<- error, | ||
frames []Frame, | ||
) { | ||
|
||
for jr := range jobChan { | ||
i := jr.Index | ||
k := jr.FrameIndex | ||
j := rb.ReverseBitsLimited(uint32(g.NumChunks), uint32(i)) | ||
ys := polyEvals[g.ChunkLength*i : g.ChunkLength*(i+1)] | ||
err := rb.ReverseBitOrderFr(ys) | ||
if err != nil { | ||
results <- err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. I used "return", because if anything that has error, the entire MakeFrames becomes false. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better to use Continuing ensures that the workers continue to consume the requests. Obviously, you could optimize this further if an error was likely here, but this would be a programming error so I don't think it needs to be optimized. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. good call. |
||
continue | ||
} | ||
coeffs, err := g.GetInterpolationPolyCoeff(ys, uint32(j)) | ||
if err != nil { | ||
results <- err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same above |
||
continue | ||
} | ||
|
||
frames[k].Coeffs = coeffs | ||
} | ||
|
||
results <- nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be inside the for loop? It'll be then one result per JobRequest There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there are "numWorker" of threads, and "NumChunks" of jobs. If it is moved inside the for loop, the worker is terminated |
||
|
||
} |
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.
No need to convert to uint64, since L83 makes the conversion already.
Also it probably has no need to use that many workers.
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.
it is assigned to number chunks at L84