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

db: optimize SeekGE/SeekLT for the noop case #1067

Merged
merged 1 commit into from
Feb 19, 2021

Conversation

sumeerbhola
Copy link
Collaborator

This is especially useful for sparse parts of the
key space which may have large numbers of deleted
versions, as happens with the locks in CockroachDB.

Results from a CockroachDB benchmark are included
below. The separated=true case is what mainly benefits
from this. The actual new numbers are listed after
the diff.

name                                                                      old time/op    new time/op    delta
ScanAllIntentDeleted/separated=false/versions=200/percent-flushed=0-16      63.8µs ± 0%    50.2µs ± 0%   -21.27%
ScanAllIntentDeleted/separated=false/versions=200/percent-flushed=50-16     24.4µs ± 0%    24.8µs ± 0%    +1.81%
ScanAllIntentDeleted/separated=false/versions=200/percent-flushed=90-16     5.60µs ± 0%    4.28µs ± 0%   -23.68%
ScanAllIntentDeleted/separated=false/versions=200/percent-flushed=100-16    1.59µs ± 0%    1.07µs ± 0%   -33.04%
ScanAllIntentDeleted/separated=true/versions=200/percent-flushed=0-16       51.7ms ± 0%     0.0ms ± 0%   -99.99%
ScanAllIntentDeleted/separated=true/versions=200/percent-flushed=50-16      14.7ms ± 0%     0.0ms ± 0%   -99.99%
ScanAllIntentDeleted/separated=true/versions=200/percent-flushed=90-16      1.04ms ± 0%    0.00ms ± 0%   -99.85%
BenchmarkScanAllIntentDeleted/separated=false/versions=200/percent-flushed=0-16         	   23816	     50222 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=false/versions=200/percent-flushed=50-16        	   46825	     24840 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=false/versions=200/percent-flushed=90-16        	  304212	      4277 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=false/versions=200/percent-flushed=100-16       	 1342129	      1066 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=true/versions=200/percent-flushed=0-16          	  197700	      5267 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=true/versions=200/percent-flushed=50-16         	  673788	      1696 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=true/versions=200/percent-flushed=90-16         	  798824	      1530 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=true/versions=200/percent-flushed=100-16        	 1256680	      1027 ns/op	       0 B/op	       0 allocs/op

This is especially useful for sparse parts of the
key space which may have large numbers of deleted
versions, as happens with the locks in CockroachDB.

Results from a CockroachDB benchmark are included
below. The separated=true case is what mainly benefits
from this. The actual new numbers are listed after
the diff.
name                                                                      old time/op    new time/op    delta
ScanAllIntentDeleted/separated=false/versions=200/percent-flushed=0-16      63.8µs ± 0%    50.2µs ± 0%   -21.27%
ScanAllIntentDeleted/separated=false/versions=200/percent-flushed=50-16     24.4µs ± 0%    24.8µs ± 0%    +1.81%
ScanAllIntentDeleted/separated=false/versions=200/percent-flushed=90-16     5.60µs ± 0%    4.28µs ± 0%   -23.68%
ScanAllIntentDeleted/separated=false/versions=200/percent-flushed=100-16    1.59µs ± 0%    1.07µs ± 0%   -33.04%
ScanAllIntentDeleted/separated=true/versions=200/percent-flushed=0-16       51.7ms ± 0%     0.0ms ± 0%   -99.99%
ScanAllIntentDeleted/separated=true/versions=200/percent-flushed=50-16      14.7ms ± 0%     0.0ms ± 0%   -99.99%
ScanAllIntentDeleted/separated=true/versions=200/percent-flushed=90-16      1.04ms ± 0%    0.00ms ± 0%   -99.85%

BenchmarkScanAllIntentDeleted/separated=false/versions=200/percent-flushed=0-16         	   23816	     50222 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=false/versions=200/percent-flushed=50-16        	   46825	     24840 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=false/versions=200/percent-flushed=90-16        	  304212	      4277 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=false/versions=200/percent-flushed=100-16       	 1342129	      1066 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=true/versions=200/percent-flushed=0-16          	  197700	      5267 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=true/versions=200/percent-flushed=50-16         	  673788	      1696 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=true/versions=200/percent-flushed=90-16         	  798824	      1530 ns/op	       0 B/op	       0 allocs/op
BenchmarkScanAllIntentDeleted/separated=true/versions=200/percent-flushed=100-16        	 1256680	      1027 ns/op	       0 B/op	       0 allocs/op
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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.

Nice.

:LGTM:

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @itsbilal and @petermattis)

Copy link
Member

@itsbilal itsbilal 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 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jbowens and @petermattis)

@sumeerbhola
Copy link
Collaborator Author

TFTR!
stressmeta has been running fine for 1h. I will merge this and continue running it.

@sumeerbhola
Copy link
Collaborator Author

@jbowens I used your suggestion, from a previous optimization PR, of replacing lastPositioningOpIsSeekPrefixGE with an enum.

@sumeerbhola sumeerbhola merged commit 959663f into cockroachdb:master Feb 19, 2021
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this pull request Feb 22, 2021
560af6e1 db: fix bug in seek noop optimization and add more testing

Release note: None

Release justification: A small bug fix to a Pebble change that
was reverted earlier this day. The Pebble change significantly
improves performance with separated intents as indicated in
the original Pebble PR
cockroachdb/pebble#1067 and is a
prerequisite for enabling separated intents (Category 2 or 4)
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this pull request Feb 26, 2021
It stresses the case where many versions were written
transactionally and all were resolved.

This is the benchmark mentioned in
cockroachdb/pebble#1067
to justify the Pebble seek noop optimization.

Release justification: Non-production code change that
adds a benchmark.

Release note: None
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this pull request Mar 1, 2021
It stresses the case where many versions were written
transactionally and all were resolved.

This is the benchmark mentioned in
cockroachdb/pebble#1067
to justify the Pebble seek noop optimization.

Release justification: Non-production code change that
adds a benchmark.

Release note: None
craig bot pushed a commit to cockroachdb/cockroach that referenced this pull request Mar 1, 2021
61037: streamingest: add AOST to stream ingestion job r=adityamaru,dt a=pbardea

To be rebased on the PR that introduces the core changefeed stream client.


Release justification: low-risk (very experimental feature), high reward
(AOST stream ingestion)

Release note: None

61197: execinfra: fix starting of processors r=yuzefovich a=yuzefovich

Almost all processors embed `ProcessorBase` and call `StartInternal` on
it. That function derives a new context and starts a tracing span if the
tracing is enabled on the parent ctx and the derived context is
returned. Previously, in all callsites the inputs to the processors
would get `Start`ed before `StartInternal` is called, but this doesn't
make sense: the consumer's ctx and span should be encompassing the
producer's (input's) ctx and span, so this commit switches all callsites
to the following layout
```
  ctx = p.StartInternal(ctx)
  input.Start(ctx) // if there are any inputs
```

Release justification: bug fix.

Release note: None

61198: storage: add a scan benchmark with resolved intents r=sumeerbhola a=sumeerbhola

It stresses the case where many versions were written
transactionally and all were resolved.

This is the benchmark mentioned in
cockroachdb/pebble#1067
to justify the Pebble seek noop optimization.

Release justification: Non-production code change that
adds a benchmark.

Release note: None

Co-authored-by: Paul Bardea <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: sumeerbhola <[email protected]>
@sumeerbhola sumeerbhola deleted the seekgelt branch March 4, 2021 21:14
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.

4 participants