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

testing(lib/trie): change lib/trie slow and intensive benchmark-oriented tests to benchmarks #2031

Merged
merged 3 commits into from
Nov 16, 2021

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Nov 12, 2021

Changes

  • Why?
    • This must be slowing down the CI quite badly
    • This makes my CPU melt when running them and forgetting to t.Skip() them
    • These are really benchmarks, and not unit testing anything really as far as i know
  • Change unit tests to a merged Go benchmark (you can run them with go test -bench)
  • Use deepcopy to copy the trie instead of re-generating one, which takes time
  • Use math/rand instead of crypto/rand to generate trie values faster. I don't think we care about crypto secured randomness in most unit tests, and especially not in a benchmark

Tests

go test -bench -run ^Benchmark_Trie_Hash$ github.com/ChainSafe/gossamer/lib/trie

Issues

Primary Reviewer

@codecov
Copy link

codecov bot commented Nov 12, 2021

Codecov Report

Merging #2031 (0ac0fd5) into development (0bc4bf1) will increase coverage by 0.16%.
The diff coverage is 61.20%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #2031      +/-   ##
===============================================
+ Coverage        59.94%   60.10%   +0.16%     
===============================================
  Files              185      193       +8     
  Lines            26343    26525     +182     
===============================================
+ Hits             15790    15942     +152     
- Misses            8677     8699      +22     
- Partials          1876     1884       +8     
Flag Coverage Δ
unit-tests 60.10% <61.20%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/gossamer/profile.go 0.00% <0.00%> (ø)
cmd/gossamer/toml_config.go 27.27% <0.00%> (ø)
dot/config.go 26.76% <0.00%> (-1.46%) ⬇️
dot/network/message.go 69.26% <0.00%> (-2.49%) ⬇️
dot/network/sync.go 5.26% <0.00%> (ø)
dot/network/utils.go 57.89% <0.00%> (ø)
dot/rpc/subscription/listeners.go 71.92% <0.00%> (ø)
dot/state/block.go 43.52% <0.00%> (-0.11%) ⬇️
dot/state/bloom.go 0.00% <0.00%> (ø)
dot/state/offline_pruner.go 0.00% <0.00%> (ø)
... and 93 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dde989d...0ac0fd5. Read the comment docs.

Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

is there a unit test for the parallel trie hashing? we should keep a unit test for it somewhere, but this change is fine

@qdm12
Copy link
Contributor Author

qdm12 commented Nov 16, 2021

is there a unit test for the parallel trie hashing? we should keep a unit test for it somewhere, but this change is fine

I'll add one in #2009 since I changed/improved the implementation for the parallel hashing.

@qdm12 qdm12 merged commit c5397c4 into development Nov 16, 2021
@qdm12 qdm12 deleted the qdm12-trie-benchmarks branch November 16, 2021 15:53
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

timwu20 pushed a commit to timwu20/gossamer that referenced this pull request Dec 6, 2021
…ented tests to benchmarks (ChainSafe#2031)

- Change unit tests to a merged Go benchmark (you can run them with `go test -bench`)
- Use deepcopy to copy the trie instead of re-generating one, which takes time
- Use math/rand instead of crypto/rand to generate trie values faster
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.

3 participants