[2/N][zero serialization] Make Batcher operate on chunks without ser/deser #700
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jianoaix
changed the title
jianoaix
changed the title
jianoaix
commented
Aug 14, 2024
| @@ -160,7 +160,7 @@ func TestBatchTrigger(t *testing.T) { | |||
| assert.Nil(t, err) | |||
| count, size := encodingStreamer.EncodedBlobstore.GetEncodedResultSize() | |||
| assert.Equal(t, count, 1) | |||
| assert.Equal(t, size, uint64(16384)) | |||
| assert.Equal(t, size, uint64(26630)) | |||
There was a problem hiding this comment.
Occasional data point about the data size reduction with the new chunk encoding with Gnark(that's separately done): the 16384 is the theoretical size (4 bytes/symbol * numSymbols in the chunk), and 26630 is the number of bytes from Gob encoding. The new chunk encoding will achieve the former.
bxue-l2
reviewed
Aug 17, 2024
disperser/batcher/batcher_test.go
Outdated
Comment on lines
230
to
231
| assert.Equal(t, uint64(27631), size) // Robert checks it | ||
|
|
bxue-l2
reviewed
Aug 17, 2024
node/grpc/server_load_test.go
Outdated
| Bundles: make(core.Bundles), | ||
| blobMessagesByOp[opID] = append(blobMessagesByOp[opID], &core.EncodedBlobMessage{ | ||
| BlobHeader: blobHeaders[i], | ||
| EncodedBundles: make(map[core.QuorumID]*core.ChunksData), |
bxue-l2
approved these changes
Aug 17, 2024
bxue-l2
approved these changes
Aug 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
This eliminates both the deserialization and serialization of chunks at the Batcher.
This matters because the cost on just the serialization of chunks can be significant: e.g. a large operator took 61s to serialize the dispersal request
Such perf/cost is on the critical path of E2E flow, so by eliminating them (as well as eliminating the cost to deserialize chunks from Encoder) will contribute directly the E2E perf.
Testing
In addition to the unit test, also tested it in Preprod, with following setup
And the Batcher was tested in these two cases:
This shows the compatibility/safety of:
Checks