Skip to content

Code cleanups #3416

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

Merged
merged 6 commits into from
Sep 26, 2024
Merged

Code cleanups #3416

merged 6 commits into from
Sep 26, 2024

Conversation

rkuris
Copy link
Contributor

@rkuris rkuris commented Sep 25, 2024

  • Added _ for large numbers for readability
  • Added some comments, removed obvious ones
  • Move prometheus stats (into stats struct)
  • Added error handling for os.Getwd() et al
  • Switch to fmt.Errorf and printf at caller
  • use 'range INT` instead of usual for loop

Why this should be merged

Cleaner code for blog post

How this works

  • Large numbers support _ as placeholders to improve readability link
  • Having variables grouped in a struct idenfies them as soley metrics for readability (metrics.batches vs batchCounter)
  • Although os.Getwd is unlikely to fail, it isn't impossible, and it's best practice to at least panic if it fails
  • fmt.Errorf returns an error, which can then be returned to the caller link
  • range over integer is easier to read and functionally equivalent link

How this was tested

Ran bench locally, still seems to work:

➜  benchmarks_eth git:(rkuris/bench_merkledb) go run . --n 1000 --v
Initializing database. done!
Generated and inserted 0 batches of size 10000 in 4.153792ms
Completed initialization with hash of 0x788272ac157b96c5af259c14657cfa0f5b261c4d07fac7cbdfba53148ad0960e
delete rate [167]       update rate [289]       insert rate [585]       batch rate [47]
delete rate [117]       update rate [259]       insert rate [417]       batch rate [36]

 - Added _ for large numbers for readability
 - Added some comments, removed obvious ones
 - Move prometheus stats (into stats struct)
 - Added error handling for os.Getwd() et al
 - Switch to fmt.Errorf and printf at caller
 - use 'range INT` instead of usual for loop
Changes:
 - Avoid calling `trieDb.Commit` until the end of run
 - Move all caching configs to separate const section
 - Beginning blockHeight is now 1, same as unit tests
 - renamed var firewoodNumberOfRevisions -> benchmark
 - Update benchmarkNumberOfRevisions to 128 (was 120)
 - Readability: units.GiB, etc from avalanchego/utils

TODOs added:
 - trieDb.Commit is still required or the bench fails
 - openFilesCacheCapacity is set to 200, not sure why
@rkuris rkuris requested a review from tsachiherman September 26, 2024 00:39
@tsachiherman tsachiherman merged commit 4602deb into tsachi/bench_merkledb Sep 26, 2024
17 of 19 checks passed
@tsachiherman tsachiherman deleted the rkuris/bench_merkledb branch September 26, 2024 03:19
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