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

Results from profiling slashing.BeginBlocker #5454

Closed
wants to merge 5 commits into from

Conversation

dshulyak
Copy link
Contributor

@dshulyak dshulyak commented Dec 27, 2019

results related to #4977

I found right knobs and made slashing.BeginBlocker work in a similar way how it works in real scenarios, it is somewhat hacky, but profile picture resembles one reported by @alexanderbez

As it was noted in one of the issues, majority of time is spent due to read amplifications in iavl store. I tried approach similar to one proposed by @ValarDragon , but i kept it simple and instead of partitioning bit array i basically created one per validator. Considered that array is limited by the size of sliding window (10000 bits or even bigger?) and we need only one per validator - this is >~1 mb per 1000 validators. I also collected db sizes after benchmark, with constant number of iterations. and storing single bitarray is clearly beneficial, unless i missed something :)

Before proceeding with this pr i will try to sync mainnet with this change (tests are passing so i assume nothing is broken) and see if this change has an expected effect.
Until then this is mostly for any early feedback, as it is highly appreciated.

Pasting results from profile.md, and there are two weblists for handleValidatorSignature attached to this PR, one before changes and one after.

slashing profiling results

original bench

benchmark

goos: linux
goarch: amd64
pkg: github.com/cosmos/cosmos-sdk/x/slashing
BenchmarkBeginBlocker100-8           100          30795066 ns/op         7486165 B/op      90541 allocs/op
PASS
ok      github.com/cosmos/cosmos-sdk/x/slashing	5.915s

db size

14M	/tmp/bench-slashing111093085/test.db
14M	/tmp/bench-slashing111093085

missed array stored as a single batch

benchcmp to original

benchmark                      old ns/op     new ns/op     delta
BenchmarkBeginBlocker100-8     30795066      15535438      -49.55%

benchmark                      old allocs     new allocs     delta
BenchmarkBeginBlocker100-8     90541          49348          -45.50%

benchmark                      old bytes     new bytes     delta
BenchmarkBeginBlocker100-8     7486165       3915672       -47.69%

db size

2,3M	/tmp/bench-slashing347257969/test.db
2,3M	/tmp/bench-slashing347257969

reuse SignedBlocksWindow and MinSignedBlocksWindow

benchcmp to original

benchmark                      old ns/op     new ns/op     delta
BenchmarkBeginBlocker100-8     30795066      13009027      -57.76%

benchmark                      old allocs     new allocs     delta
BenchmarkBeginBlocker100-8     90541          43555          -51.89%

benchmark                      old bytes     new bytes     delta
BenchmarkBeginBlocker100-8     7486165       3654145       -51.19%

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

func benchmarkBeginBlocker(b *testing.B, num int) {
params := DefaultParams()
params.SignedBlocksWindow = 500
ctx, _, _, _, keeper, store, _ := slashingkeeper.CreateTestInputStore(b, params)
Copy link
Contributor

Choose a reason for hiding this comment

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

declaration has 4 blank identifiers (from dogsled)

@codecov
Copy link

codecov bot commented Dec 27, 2019

Codecov Report

Merging #5454 into master will decrease coverage by 0.27%.
The diff coverage is 56.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5454      +/-   ##
==========================================
- Coverage   54.51%   54.24%   -0.28%     
==========================================
  Files         318      320       +2     
  Lines       19131    19332     +201     
==========================================
+ Hits        10430    10486      +56     
- Misses       7915     8058     +143     
- Partials      786      788       +2
Impacted Files Coverage Δ
x/slashing/internal/keeper/signing_info.go 68.42% <100%> (+0.85%) ⬆️
x/slashing/abci.go 100% <100%> (ø) ⬆️
x/slashing/internal/keeper/infractions.go 28.42% <41.22%> (-53.25%) ⬇️
x/slashing/internal/types/votearray.go 63.63% <63.63%> (ø)
x/slashing/internal/types/bitarray.go 85.71% <85.71%> (ø)
x/mock/app.go 62.83% <0%> (-1.36%) ⬇️

@dshulyak
Copy link
Contributor Author

dshulyak commented Jan 2, 2020

closing it in favor of #5461. not included changes will be separate PRs

@dshulyak dshulyak closed this Jan 2, 2020
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.

2 participants