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

Improve cost of BLS key serialization for gRPC #1343

Merged
merged 6 commits into from
Apr 19, 2023
Merged

Conversation

hexfusion
Copy link
Contributor

@hexfusion hexfusion commented Apr 13, 2023

Note: this will require a protocol version bump

Why this should be merged

This PR is a followup to #1326 where we benchmarked the serialzation costs of BLS public keys. The profile from bench showed that the majority of the cost was spent in CGO on very expensive operations including Compressing, Decompressing bytes and re-verifying the keys. Verification was not explicitly required because the keys were already verified before being added to the P-Chain.

before

$ go test -benchmem -run=^$ -bench ^BenchmarkGetValidatorSet$ github.com/ava-labs/avalanchego/snow/validators/gvalidators -memprofile benchvset.mem -cpuprofile benchvset.cpu

goos: linux
goarch: amd64
pkg: github.com/ava-labs/avalanchego/snow/validators/gvalidators
cpu: Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz
BenchmarkGetValidatorSet
BenchmarkGetValidatorSet/get_validator_set_1_validators
BenchmarkGetValidatorSet/get_validator_set_1_validators-12                  2175            534900 ns/op           10057 B/op        190 allocs/op
BenchmarkGetValidatorSet/get_validator_set_16_validators
BenchmarkGetValidatorSet/get_validator_set_16_validators-12                  350           3139479 ns/op           21040 B/op        300 allocs/op
BenchmarkGetValidatorSet/get_validator_set_32_validators
BenchmarkGetValidatorSet/get_validator_set_32_validators-12                  204           5763480 ns/op           33196 B/op        414 allocs/op
BenchmarkGetValidatorSet/get_validator_set_1024_validators
BenchmarkGetValidatorSet/get_validator_set_1024_validators-12                  7         156047375 ns/op          794016 B/op       7437 allocs/op
BenchmarkGetValidatorSet/get_validator_set_2048_validators
BenchmarkGetValidatorSet/get_validator_set_2048_validators-12                  4         310664506 ns/op         1607208 B/op      14673 allocs/op

after

$ go test -benchmem -run=^$ -bench ^BenchmarkGetValidatorSet$ github.com/ava-labs/avalanchego/snow/validators/gvalidators -memprofile benchvset.mem -cpuprofile benchvset.cpu

goos: linux
goarch: amd64
pkg: github.com/ava-labs/avalanchego/snow/validators/gvalidators
cpu: Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz
BenchmarkGetValidatorSet
BenchmarkGetValidatorSet/get_validator_set_1_validators
BenchmarkGetValidatorSet/get_validator_set_1_validators-12                  4336            264489 ns/op           10204 B/op        190 allocs/op
BenchmarkGetValidatorSet/get_validator_set_16_validators
BenchmarkGetValidatorSet/get_validator_set_16_validators-12                 2874            385106 ns/op           23575 B/op        299 allocs/op
BenchmarkGetValidatorSet/get_validator_set_32_validators
BenchmarkGetValidatorSet/get_validator_set_32_validators-12                 2376            494729 ns/op           38050 B/op        412 allocs/op
BenchmarkGetValidatorSet/get_validator_set_1024_validators
BenchmarkGetValidatorSet/get_validator_set_1024_validators-12                249           4549176 ns/op          959468 B/op       7385 allocs/op
BenchmarkGetValidatorSet/get_validator_set_2048_validators
BenchmarkGetValidatorSet/get_validator_set_2048_validators-12                142           8424447 ns/op         1917264 B/op      14573 allocs/op

result: ~97% reduction in CPU time.

profiles

benchvset.cpu.txt
benchvset.mem.txt

Closes #1327

How this works

Remove and replace unnecessarily expensive calls to CGO with less expensive variants.

How this was tested

manually

@hexfusion hexfusion added the DO NOT MERGE This PR must not be merged in its current state label Apr 14, 2023
Signed-off-by: Sam Batschelet <sam.batschelet@avalabs.org>
Signed-off-by: Sam Batschelet <sam.batschelet@avalabs.org>
@hexfusion hexfusion changed the title Improve cost of BLS key serialization Improve cost of BLS key serialization for gRPC Apr 14, 2023
@hexfusion hexfusion self-assigned this Apr 14, 2023
Signed-off-by: Sam Batschelet <sam.batschelet@avalabs.org>
@hexfusion hexfusion removed the DO NOT MERGE This PR must not be merged in its current state label Apr 14, 2023
@hexfusion hexfusion requested a review from gyuho as a code owner April 14, 2023 11:40
Signed-off-by: Sam Batschelet <sam.batschelet@avalabs.org>
@hexfusion hexfusion added sdk This involves SDK tooling or frameworks benchmarking labels Apr 14, 2023
Copy link
Contributor

@abi87 abi87 left a comment

Choose a reason for hiding this comment

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

lgtm, there may be other places in the codebase where we can use this optimization (matter for a different PR)

@hexfusion
Copy link
Contributor Author

lgtm, there may be other places in the codebase where we can use this optimization (matter for a different PR)

Yeah I was going to break out the logic into funcs my fear is someone using the it in the wrong place (like me :)) to grab a perf gain and opening vuln.

@StephenButtolph StephenButtolph merged commit 2aa256b into dev Apr 19, 2023
@StephenButtolph StephenButtolph deleted the bls_perf branch April 19, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sdk This involves SDK tooling or frameworks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants