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.

Sign up for GitHub

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

Jump to bottom

feat: Add benchmark for store read/write #5312

Merged
merged 8 commits into from
Nov 24, 2021
Merged

feat: Add benchmark for store read/write #5312

merged 8 commits into from
Nov 24, 2021

Conversation

pmnoxx
Copy link
Contributor

@pmnoxx pmnoxx commented Nov 17, 2021 •
edited
Loading

Add benchmark for reading / writing from store.

See #4771

cd chain/network/benches
rm -rf /tmp/xxxxx ; cargo bench benchmark_db

TODO:

  • Add tempdir
  • Pregenerate keys
  • Add stats to see number of entries read successfully
  • Why we can only read 48.5% entries back when we write them to DB?
  • Move code to a better place
  • Document what the benchmark does

@pmnoxx pmnoxx force-pushed the piotr-add-store-db-bnenchmark branch from 8d5ca6a to 15a0640 Compare November 17, 2021 07:06
@pmnoxx pmnoxx mentioned this pull request Nov 17, 2021
Closed
@pmnoxx pmnoxx changed the title Add benchmark for store read/write feat: Add benchmark for store read/write Nov 17, 2021
matklad
matklad reviewed Nov 17, 2021
View reviewed changes
chain/network/benches/ibf.rs Outdated Show resolved Hide resolved
chain/network/benches/ibf.rs Outdated Show resolved Hide resolved
@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 17, 2021

@matklad For some reason I'm only able to get back 48.5% of entries I put in the database. That seems wrong.

@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 17, 2021

@matklad use tempfile::TempDir is too old. It hasn't been updated for 3 years and it uses old random. We will have to find another package.

@matklad
Copy link
Contributor

matklad commented Nov 17, 2021

https://crates.io/crates/tempfile is what we should use here. https://crates.io/crates/tempdir is indeed the old deprecated thing.

@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 17, 2021

@matklad I'm not sure this belongs to chain/network/benches/ibf.rs. What would be the best place to put it?

@pmnoxx pmnoxx force-pushed the piotr-add-store-db-bnenchmark branch from d3e5de2 to 76a6261 Compare November 18, 2021 04:08
@pmnoxx pmnoxx requested a review from matklad November 18, 2021 04:08
@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 18, 2021

@mm-near I refactored the benchmark, can you take another look?

@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 19, 2021

@djsatok Figured out the reason. It turns out that, we set flags for this column for reference couting. That's the reason why not all columns get written to database. Thanks for looking into this.

@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 19, 2021

@matklad Ok, the benchmark is ready.

Longarithm
Longarithm approved these changes Nov 19, 2021
View reviewed changes
Copy link
Member

@Longarithm Longarithm left a comment

Choose a reason for hiding this comment

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

That's great!
Can we also make separate benchmark for ColState writing values like this one, maybe in other PR?
I think ideally we need to check performance for each column with separately set options.

@pmnoxx pmnoxx self-assigned this Nov 19, 2021
@pmnoxx pmnoxx changed the title feat: Add benchmark for store read/write feat: Add benchmark for store read/write Nov 20, 2021
@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 20, 2021

@mm-near Can you take a look?

@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 20, 2021

That's great! Can we also make separate benchmark for ColState writing values like this one, maybe in other PR? I think ideally we need to check performance for each column with separately set options.

Good idea, we can do that in another PR. It should be easy to modify this code as an example.

@pmnoxx pmnoxx added the P-low Priority: low label Nov 21, 2021
bowenwang1996
bowenwang1996 reviewed Nov 22, 2021
View reviewed changes
core/store/benches/store_bench.rs Outdated Show resolved Hide resolved
core/store/benches/store_bench.rs Outdated Show resolved Hide resolved
core/store/benches/store_bench.rs Outdated Show resolved Hide resolved
@frol frol removed their request for review November 23, 2021 12:57
mm-near
mm-near approved these changes Nov 24, 2021
View reviewed changes
Copy link
Contributor

@mm-near mm-near left a comment

Choose a reason for hiding this comment

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

What were the results of your benchmark? are they stable between runs?

core/store/benches/store_bench.rs Outdated Show resolved Hide resolved
@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 24, 2021

@bowenwang1996 Please, take a look.

@pmnoxx pmnoxx force-pushed the piotr-add-store-db-bnenchmark branch from c73643f to b005f33 Compare November 24, 2021 21:12
@pmnoxx pmnoxx force-pushed the piotr-add-store-db-bnenchmark branch from b005f33 to 56ef270 Compare November 24, 2021 23:13
@near-bulldozer near-bulldozer bot merged commit 21c760d into master Nov 24, 2021
@near-bulldozer near-bulldozer bot deleted the piotr-add-store-db-bnenchmark branch November 24, 2021 23:49
pmnoxx added a commit that referenced this pull request Nov 25, 2021
@pmnoxx
feat: Add benchmark for store read/write (#5312)
fcb6c61 
Add benchmark for reading / writing from store.

See #4771

```
cd chain/network/benches
rm -rf /tmp/xxxxx ; cargo bench benchmark_db
```

TODO:
- [x] Add tempdir
- [x] Pregenerate keys
- [x] Add stats to see number of entries read successfully
- [x] Why we can only read 48.5% entries back when we write them to DB?
- [x] Move code to a better place
- [x] Document what the benchmark does
@pmnoxx pmnoxx added A-benchmark Area: performance benchmarks A-chain Area: Chain, client & related A-core C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Dec 5, 2021
@Longarithm Longarithm mentioned this pull request Dec 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mm-near mm-near mm-near approved these changes

@Longarithm Longarithm Longarithm approved these changes

@matklad matklad Awaiting requested review from matklad

@bowenwang1996 bowenwang1996 Awaiting requested review from bowenwang1996

Assignees

@pmnoxx pmnoxx

Labels
A-benchmark Area: performance benchmarks A-chain Area: Chain, client & related C-enhancement Category: An issue proposing an enhancement or a PR with one. P-low Priority: low
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pmnoxx @matklad @Longarithm @bowenwang1996 @mm-near