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

Migrate bench to a cobra style command #680

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ivanvc
Copy link
Member

@ivanvc ivanvc commented Jan 24, 2024

Migrates bbolt bench to a cobra-style command. I made some changes to clean up the code a bit.

  1. Even though the functions in the original code belonged to a struct, most of the arguments were being forwarded to the nested functions, so I changed the struct to store these values and have them available in the functions
  2. Even though there was a read-mode option, the only available mode was sequential (seq), So I removed this option.

    bbolt/cmd/bbolt/main.go

    Lines 1313 to 1323 in 019c34e

    switch options.ReadMode {
    case "seq":
    switch options.WriteMode {
    case "seq-nest", "rnd-nest":
    err = cmd.runReadsSequentialNested(db, options, results)
    default:
    err = cmd.runReadsSequential(db, options, results)
    }
    default:
    return fmt.Errorf("invalid read mode: %s", options.ReadMode)
    }

    While having these changes, the output of the command and the behavior remain the same.

Old run:

$ bbolt bench -write-mode rnd-nest -batch-size 500 
starting write benchmark.
Starting write iteration 0
Finished write iteration 0
Starting write iteration 500
Finished write iteration 500
starting read benchmark.
# Write 1000(ops)       1.53224ms       (1.532µs/op)    (652741 op/sec)
# Read  57310000(ops)   1.000015207s    (17ns/op)       (58823529 op/sec)

New:

$ bbolt bench --write-mode rnd-nest --batch-size 500 
starting write benchmark.
Starting write iteration 0
Finished write iteration 0
Starting write iteration 500
Finished write iteration 500
starting read benchmark.
# Write 1000(ops)       1.570807ms      (1.57µs/op)     (636942 op/sec)
# Read  50249000(ops)   1.000010436s    (19ns/op)       (52631578 op/sec)

Old:

$ bbolt bench -h
Usage:
  -batch-size int
    
  -blockprofile string
    
  -count int
         (default 1000)
  -cpuprofile string
    
  -fill-percent float
         (default 0.5)
  -key-size int
         (default 8)
  -memprofile string
    
  -no-sync
    
  -path string
    
  -profile-mode string
         (default "rw")
  -read-mode string
         (default "seq")
  -value-size int
         (default 32)
  -work
    
  -write-mode string
         (default "seq")
flag: help requested
exit status 1

New:

$ bbolt bench -h
run synthetic benchmark against bbolt

Usage:
  bbolt bench [flags]

Flags:
      --batch-size uint32      the step size for each iteration, if not provided iteration size is used, it needs to be evenly divided by the iteration count (count)
      --blockprofile string   output file for the pprof block profile
      --count uint32           the number of iterations (default 1000)
      --cpuprofile string     output file for the pprof CPU profile
      --fill-percent float     the percentage that split pages are filled (default 0.5)
  -h, --help                   help for bench
      --key-size int           the size for the key, from the key value insertion (default 8)
      --memprofile string     output file for the pprof memoery profile
      --no-sync                skip fsync() calls after each commit
      --path string            path to the database file
      --profile-mode string    the profile mode to execute, valid modes are r, w and rw (default "rw")
      --value-size int         the size for the value, from the key value insertion (default 32)
      --work                   if set, the database path won't be removed after the execution
      --write-mode string      the write mode, valid values are seq, rnd, seq-nest and rnd-nest (default "seq")

Relates to #472.

@ivanvc ivanvc force-pushed the migrate-bench-to-cobra-style-command-using-bench-exec-struct branch from 353ed32 to 0a529bb Compare January 29, 2024 23:06
@ivanvc ivanvc force-pushed the migrate-bench-to-cobra-style-command-using-bench-exec-struct branch from 0a529bb to 902557d Compare February 21, 2024 05:28
@ahrtr
Copy link
Member

ahrtr commented Mar 27, 2024

A quick question, @ivanvc is there any user-facing change? If there isn't such change, then we should be good to merge this PR.

@ivanvc
Copy link
Member Author

ivanvc commented Mar 27, 2024

@ahrtr, I deleted the -read-mode flag, as the only option available was seq. Providing any other value halted the execution.

However, changing from built-in Go flags to Cobra flags isn't backward compatible, as Go flags aren't POSIX compliant.

@ahrtr
Copy link
Member

ahrtr commented Mar 27, 2024

OK, thanks for the clarification. Then let's merge this PR once we cut release-1.4 branch.

@ivanvc
Copy link
Member Author

ivanvc commented Apr 11, 2024

Drafting because of #711

@github-actions github-actions bot added the stale label Aug 6, 2024
@k8s-ci-robot
Copy link

@ivanvc: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-bbolt-test-2-cpu-arm64 902557d link true /test pull-bbolt-test-2-cpu-arm64
pull-bbolt-test-4-cpu-arm64 902557d link true /test pull-bbolt-test-4-cpu-arm64
pull-bbolt-test-4-cpu-race-arm64 902557d link true /test pull-bbolt-test-4-cpu-race-arm64
pull-bbolt-robustness-arm64 902557d link true /test pull-bbolt-robustness-arm64

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants