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

storage: improve mvcc benchmarks under -short #115810

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

RaduBerinde
Copy link
Member

@RaduBerinde RaduBerinde commented Dec 7, 2023

storage: improve mvcc benchmarks under -short

Many of the MVCC benchmarks have too many variants and are completely
sipped when using the -short flag (which is used for the weekly
microbenchmarks job). Conversely, other benchmarks are not skipped but
still have quite a few variants.

In this commit we change relevant tests to run a small set of
configurations under -short. We also fix a few smaller issues, for
example we remove some bogus empty acquire lock benchmarks with
heldOtherTxn=true/heldSameTxn=true.

Fixes #115794
Release note: None

storage: remove batch=true variants of BenchmarkMVCCPut

This benchmark has batching variants but the batch size is b.N which
can hit the 4GB batch limit.

Given that we already have a separate MVCCBatchPut which has
reasonable batch sizes, we are removing the batched variants of
BenchmarkMVCCPut.

Fixes: #115811
Release note: None

@RaduBerinde RaduBerinde requested a review from a team as a code owner December 7, 2023 19:34
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jbowens and @sumeerbhola)


pkg/storage/bench_pebble_test.go line 330 at r1 (raw file):

	var testCases []testCase
	for _, batch := range []bool{false, true} {
		for _, numVersions := range []int{1, 10, 100} {

Note: here I am restoring the variants which were removed in #99726

@RaduBerinde RaduBerinde added backport-23.2.x Flags PRs that need to be backported to 23.2. backport-23.2.0-rc labels Dec 7, 2023
@RaduBerinde
Copy link
Member Author

Added a commit that fixes #115811.

Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten, @RaduBerinde, and @sumeerbhola)


-- commits line 5 at r4:
nit: "skipped when"


pkg/storage/bench_pebble_test.go line 330 at r1 (raw file):

Previously, RaduBerinde wrote…

Note: here I am restoring the variants which were removed in #99726

Oops, thanks

Many of the MVCC benchmarks have too many variants and are completely
skipped when using the `-short` flag (which is used for the weekly
microbenchmarks job). Conversely, other benchmarks are not skipped but
still have quite a few variants.

In this commit we change relevant tests to run a small set of
configurations under `-short`. We also fix a few smaller issues, for
example we remove some bogus empty acquire lock benchmarks with
`heldOtherTxn=true/heldSameTxn=true`.

Fixes cockroachdb#115794
Release note: None
This benchmark has batching variants but the batch size is `b.N` which
can hit the 4GB batch limit.

Given that we already have a separate `MVCCBatchPut` which has
reasonable batch sizes, we are removing the batched variants of
`BenchmarkMVCCPut`.

Fixes: cockroachdb#115811
Release note: None
Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jbowens, @nvanbenschoten, and @sumeerbhola)


-- commits line 5 at r4:

Previously, jbowens (Jackson Owens) wrote…

nit: "skipped when"

Haha, I like to sip my benchmarks. Fixed.

@RaduBerinde
Copy link
Member Author

bors r+

@craig craig bot merged commit 8dccf1f into cockroachdb:master Dec 13, 2023
8 of 9 checks passed
@craig
Copy link
Contributor

craig bot commented Dec 13, 2023

Build succeeded:

Copy link

blathers-crl bot commented Dec 13, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 5ec3a5a to blathers/backport-release-23.2-115810: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.


error creating merge commit from 5ec3a5a to blathers/backport-release-23.2.0-rc-115810: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.0-rc failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storage: BenchmarkMVCCPut_Pebble panics with batch too large storage: make short version of MVCC benchmarks
3 participants