Skip to content
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

Implement GPBFT message compression using zstd #793

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Conversation

masih
Copy link
Member

@masih masih commented Dec 12, 2024

Add the ability to compress GPBFT messages controllable via manifest. Implement benchmarks to compare vanilla CBOR and ZSTD encoding.

Basic local run:

BenchmarkCborEncoding-12    	   47173	     25491 ns/op	  135409 B/op	      87 allocs/op
BenchmarkCborDecoding-12    	   64550	     18078 ns/op	   61728 B/op	     209 allocs/op
BenchmarkZstdEncoding-12    	   29061	     41489 ns/op	  193455 B/op	      88 allocs/op
BenchmarkZstdDecoding-12    	   66172	     17924 ns/op	  176517 B/op	     211 allocs/op

Fixes #786

@masih masih requested a review from Kubuxu December 12, 2024 20:20
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 56.25000% with 28 lines in your changes missing coverage. Please review.

Project coverage is 69.31%. Comparing base (1b17c6c) to head (1578660).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
msg_encoding.go 53.84% 12 Missing and 6 partials ⚠️
host.go 55.00% 5 Missing and 4 partials ⚠️
manifest/manifest.go 80.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #793      +/-   ##
==========================================
- Coverage   69.64%   69.31%   -0.33%     
==========================================
  Files          77       78       +1     
  Lines        7900     7953      +53     
==========================================
+ Hits         5502     5513      +11     
- Misses       1949     1979      +30     
- Partials      449      461      +12     
Files with missing lines Coverage Δ
manifest/manifest.go 53.75% <80.00%> (-0.45%) ⬇️
host.go 65.15% <55.00%> (-2.29%) ⬇️
msg_encoding.go 53.84% <53.84%> (ø)

... and 3 files with indirect coverage changes

@masih masih requested a review from rvagg December 12, 2024 20:58
@Stebalien
Copy link
Member

Lol, those decode benchmarks. Zstd is fast.

@masih
Copy link
Member Author

masih commented Dec 12, 2024

We can reduce allocation significantly by pooling the buffers too across both CBOR and ZSTD. but baby steps/small PRs.

msg_encoding_test.go Outdated Show resolved Hide resolved
}

func (c *zstdGMessageEncoding) Decode(v []byte) (*gpbft.GMessage, error) {
cborEncoded, err := c.decompressor.DecodeAll(v, make([]byte, 0, len(v)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of interest - have you tried variations of the dst argument here? nil or oversizing by a % given your knowledge of the likely compression ratio?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have. My plan is to introduce pooling of buffers in a separate PR as it is an optimisation. There are a few places in encoding pipeline that we can benefit from it to reduce allocations.

In terms of impact, this probably doesn't matter as much considering the heap allocation numbers from passive testing. It is just wasteful.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat

@masih masih force-pushed the masih/gmsg-compress branch from ea3ad13 to 6093105 Compare December 13, 2024 09:27
msg_encoding_test.go Outdated Show resolved Hide resolved
Add the ability to compress GPBFT messages controllable via manifest.
Implement benchmarks to compare vanilla CBOR and ZSTD encoding.

Basic local run:
```
BenchmarkCborEncoding-12    	   47173	     25491 ns/op	  135409 B/op	      87 allocs/op
BenchmarkCborDecoding-12    	   64550	     18078 ns/op	   61728 B/op	     209 allocs/op
BenchmarkZstdEncoding-12    	   29061	     41489 ns/op	  193455 B/op	      88 allocs/op
BenchmarkZstdDecoding-12    	   66172	     17924 ns/op	  176517 B/op	     211 allocs/op
```

Fixes #786
@masih masih force-pushed the masih/gmsg-compress branch from 6093105 to 1578660 Compare December 13, 2024 13:19
@masih masih enabled auto-merge December 13, 2024 13:21
@masih masih added this pull request to the merge queue Dec 13, 2024
Merged via the queue into main with commit b04ede1 Dec 13, 2024
13 of 14 checks passed
@masih masih deleted the masih/gmsg-compress branch December 13, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce GPBFT message compression
4 participants