[5/N] Chunk encoding optimization: Add support of new encoding at Node #636
Conversation
| @@ -198,7 +206,7 @@ func (b Bundle) Serialize() ([]byte, error) { | |||
| } | |||
| result := make([]byte, size+8) | |||
| buf := result | |||
| metadata := uint64(GnarkBundleEncodingFormat) | (uint64(len(b[0].Coeffs)) << 8) | |||
| metadata := (uint64(GnarkBundleEncodingFormat) << (NumBundleHeaderBits - NumBundleEncodingFormatBits)) | uint64(len(b[0].Coeffs)) | |||
There was a problem hiding this comment.
So you are putting length at lower address, what is the motivation for the change
There was a problem hiding this comment.
To make it compatible with existing chunk encoding, so that the same algo can be used to parse both old and new encoding
core/data.go
Outdated
| return nil, errors.New("invalid bundle data encoding format") | ||
| } | ||
| chunkLen := meta >> 8 | ||
| chunkLen := meta << NumBundleEncodingFormatBits >> NumBundleEncodingFormatBits |
There was a problem hiding this comment.
would be good to add (), or comment on the precedence on the binary operations are from left to right
node/grpc/utils.go
Outdated
| bundles[uint8(quorumID)] = make([]*encoding.Frame, len(chunks.GetChunks())) | ||
| for k, data := range chunks.GetChunks() { | ||
| chunk, err := new(encoding.Frame).Deserialize(data) | ||
| if len(bundle.GetBundle()) > 0 { |
There was a problem hiding this comment.
can we have a get format function, it looks a bit adhoc
There was a problem hiding this comment.
There is no field to indicate the format, it's checking bundle field and then fallback to the old chunk encoding
| if err != nil { | ||
| return nil, err | ||
| if usingBundleBytes { | ||
| rawBundle, ok := rawBundles[quorumID] |
There was a problem hiding this comment.
rawBundle will have zero-copy and hence more efficient compared to the old encoding below (which has to allocate memory to concat chunks)
core/data.go
Outdated
| return nil, errors.New("invalid bundle data encoding format") | ||
| } | ||
| chunkLen := meta >> 8 | ||
| chunkLen := meta << NumBundleEncodingFormatBits >> NumBundleEncodingFormatBits |
| @@ -198,7 +206,7 @@ func (b Bundle) Serialize() ([]byte, error) { | |||
| } | |||
| result := make([]byte, size+8) | |||
| buf := result | |||
| metadata := uint64(GnarkBundleEncodingFormat) | (uint64(len(b[0].Coeffs)) << 8) | |||
| metadata := (uint64(GnarkBundleEncodingFormat) << (NumBundleHeaderBits - NumBundleEncodingFormatBits)) | uint64(len(b[0].Coeffs)) | |||
There was a problem hiding this comment.
To make it compatible with existing chunk encoding, so that the same algo can be used to parse both old and new encoding
node/grpc/utils.go
Outdated
| if err != nil { | ||
| resultChan <- err | ||
| return | ||
| } | ||
| bundles[uint8(quorumID)][k] = chunk | ||
| bundles[uint8(quorumID)] = bundleMsg | ||
| } else { |
There was a problem hiding this comment.
nit: else if format == core.GobBundleEncodingFormat
then add else block that throws unexpected encoding format error
node/grpc/utils.go
Outdated
| if err != nil { | ||
| resultChan <- err | ||
| return | ||
| } | ||
| bundles[uint8(quorumID)][k] = chunk | ||
| bundles[uint8(quorumID)] = bundleMsg | ||
| } else { |
| for k, data := range chunks.GetChunks() { | ||
| chunk, err := new(encoding.Frame).Deserialize(data) | ||
| if format == core.GnarkBundleEncodingFormat { | ||
| bundleMsg, err := new(core.Bundle).Deserialize(bundle.GetBundle()) |
There was a problem hiding this comment.
This deserialization will be 3.6x faster (#624)
Why are these changes needed?
Support both gob and gnark encoding at Node.
Checks