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: add scan benchmark with resolved intents and fix pebbleMVCCS… #96380

Merged
merged 1 commit into from
Feb 3, 2023

Conversation

sumeerbhola
Copy link
Collaborator

…canner.itersBeforeSeek

This commit restores the lower-bound of 1 on
pebbleMVCCScanner.itersBeforeSeek. This has no effect on the added benchmark since on master (unlike v22.2) we use Pebble's Iterator.NextPrefix for the common case of stepping to the next roachpb.Key. itersBeforeSeek continues to be used for seeking to a particular version and for reverse scans.

The added benchmark has 7 levels and resolved intents where both the Set and SingleDelete of the intent are present in Pebble. It tries to trick pebbleMVCCScanner with having keys with many versions in the beginning of the scan. Benchmark results:

BenchmarkMVCCScannerWithIntentsAndVersions-10    	4000	    316177 ns/op	  133119 B/op	      28 allocs/op

stats: (interface (dir, seek, step): (fwd, 2, 1999), (rev, 0, 0)), (internal (dir, seek, step): (fwd, 2, 3110), (rev, 0, 0)), (internal-stats: (block-bytes: (total 15 K, cached 15 K)), (points: (count 3.1 K, key-bytes 88 K, value-bytes 60 K, tombstoned 0)))

Informs #96361

Epic: none

Release note: None

@sumeerbhola sumeerbhola requested a review from a team as a code owner February 1, 2023 21:30
@cockroach-teamcity
Copy link
Member

This change is Reviewable

…canner.itersBeforeSeek

This commit restores the lower-bound of 1 on
pebbleMVCCScanner.itersBeforeSeek. This has no effect on the added
benchmark since on master (unlike v22.2) we use Pebble's Iterator.NextPrefix
for the common case of stepping to the next roachpb.Key. itersBeforeSeek
continues to be used for seeking to a particular version and for reverse
scans.

The added benchmark has 7 levels and resolved intents where both the Set
and SingleDelete of the intent are present in Pebble. It tries to trick
pebbleMVCCScanner with having keys with many versions in the beginning
of the scan. Benchmark results:

BenchmarkMVCCScannerWithIntentsAndVersions-10    	4000	    316177 ns/op	  133119 B/op	      28 allocs/op

stats: (interface (dir, seek, step): (fwd, 2, 1999), (rev, 0, 0)), (internal (dir, seek, step): (fwd, 2, 3110), (rev, 0, 0)),
(internal-stats: (block-bytes: (total 15 K, cached 15 K)), (points: (count 3.1 K, key-bytes 88 K, value-bytes 60 K, tombstoned 0)))

Informs cockroachdb#96361

Epic: none

Release note: None
Copy link
Collaborator

@nicktrav nicktrav 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 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jbowens and @sumeerbhola)


-- commits line 16 at r2:
These numbers are hard to gauge as good / bad. Any comparison / delta worth putting in here? Or are we simply establishing a baseline?


pkg/storage/bench_test.go line 1930 at r2 (raw file):

		sstFile, err := eng.Create(sstFileName)
		require.NoError(b, err)
		// No improvement with v3 since the multiple versions are in different

At some point wont all files be v3? Is there any harm in using that version here? Or even just the "latest" / "newest" version?

@sumeerbhola
Copy link
Collaborator Author

Flakes in backupccl :(

Copy link
Collaborator Author

@sumeerbhola sumeerbhola 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! 1 of 0 LGTMs obtained (waiting on @jbowens and @nicktrav)


-- commits line 16 at r2:

Previously, nicktrav (Nick Travers) wrote…

These numbers are hard to gauge as good / bad. Any comparison / delta worth putting in here? Or are we simply establishing a baseline?

Don't have a delta. This is slightly faster than v22.1, which came in ~350000 ns/op.


pkg/storage/bench_test.go line 1930 at r2 (raw file):

Previously, nicktrav (Nick Travers) wrote…

At some point wont all files be v3? Is there any harm in using that version here? Or even just the "latest" / "newest" version?

It may not be the default for v23.1. Eventually we'll want to test with both.

@sumeerbhola
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 3, 2023

Build succeeded:

@craig craig bot merged commit 44d9f3c into cockroachdb:master Feb 3, 2023
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