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

kvserver/rangefeed: use RangeKeyChanged() in catchup scans #86512

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Aug 20, 2022

This avoids a key comparison in the hot path. However, we do not have
any benchmarks that emit range keys (they are always filtered out by the
MVCCIncrementalIterator), so it doesn't show up here:

name                                                                  old time/op  new time/op  delta
CatchUpScan/mixed-case/withDiff=true/perc=0.00/numRangeKeys=0-24       525ms ± 1%   520ms ± 1%    ~     (p=0.056 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=0.00/numRangeKeys=1-24       570ms ± 0%   559ms ± 0%  -1.98%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=0.00/numRangeKeys=100-24     624ms ± 1%   618ms ± 0%  -0.96%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=50.00/numRangeKeys=0-24      1.03s ± 0%   1.02s ± 0%  -0.56%  (p=0.016 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=50.00/numRangeKeys=1-24      1.21s ± 0%   1.21s ± 0%    ~     (p=0.056 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=50.00/numRangeKeys=100-24    1.71s ± 0%   1.70s ± 1%    ~     (p=0.056 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=75.00/numRangeKeys=0-24      898ms ± 0%   906ms ± 0%  +0.86%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=75.00/numRangeKeys=1-24      1.08s ± 1%   1.07s ± 1%    ~     (p=0.421 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=75.00/numRangeKeys=100-24    1.44s ± 0%   1.44s ± 0%    ~     (p=0.841 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=95.00/numRangeKeys=0-24      190ms ± 1%   189ms ± 1%    ~     (p=0.056 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=95.00/numRangeKeys=1-24      215ms ± 0%   215ms ± 1%    ~     (p=0.421 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=95.00/numRangeKeys=100-24    325ms ± 1%   324ms ± 1%    ~     (p=0.548 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=99.00/numRangeKeys=0-24      158ms ± 1%   158ms ± 1%    ~     (p=0.690 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=99.00/numRangeKeys=1-24      181ms ± 0%   182ms ± 0%  +0.73%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=99.00/numRangeKeys=100-24    294ms ± 0%   293ms ± 1%    ~     (p=0.421 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=0-24      782ms ± 0%   788ms ± 1%  +0.81%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=1-24      901ms ± 0%   906ms ± 0%  +0.55%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=100-24    918ms ± 1%   920ms ± 0%    ~     (p=0.310 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=0-24     663ms ± 1%   671ms ± 0%  +1.21%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=1-24     793ms ± 0%   799ms ± 0%  +0.68%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=100-24   803ms ± 0%   813ms ± 1%  +1.25%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=75.00/numRangeKeys=0-24     594ms ± 0%   601ms ± 0%  +1.28%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=75.00/numRangeKeys=1-24     722ms ± 0%   730ms ± 1%  +1.17%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=75.00/numRangeKeys=100-24   733ms ± 0%   742ms ± 1%  +1.20%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=0-24     172ms ± 0%   170ms ± 1%  -0.70%  (p=0.032 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=1-24     194ms ± 0%   194ms ± 1%    ~     (p=0.421 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=100-24   303ms ± 0%   303ms ± 1%    ~     (p=0.841 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=99.00/numRangeKeys=0-24     153ms ± 0%   154ms ± 0%  +0.59%  (p=0.016 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=99.00/numRangeKeys=1-24     177ms ± 1%   177ms ± 1%    ~     (p=0.690 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=99.00/numRangeKeys=100-24   288ms ± 1%   288ms ± 0%    ~     (p=0.841 n=5+5)

Release justification: bug fixes and low-risk updates to new functionality

Release note: None

@erikgrinaker erikgrinaker requested a review from tbg August 20, 2022 15:10
@erikgrinaker erikgrinaker self-assigned this Aug 20, 2022
@erikgrinaker erikgrinaker requested review from a team as code owners August 20, 2022 15:10
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@erikgrinaker erikgrinaker force-pushed the catchup-scan-rangekeychanged branch 2 times, most recently from d6551ee to 6292ddf Compare August 21, 2022 14:27
@erikgrinaker erikgrinaker requested a review from a team August 22, 2022 11:36
This avoids a key comparison in the hot path. However, we do not have
any benchmarks that emit range keys (they are always filtered out by the
`MVCCIncrementalIterator`), so it doesn't show up here:

```
name                                                                  old time/op  new time/op  delta
CatchUpScan/mixed-case/withDiff=true/perc=0.00/numRangeKeys=0-24       525ms ± 1%   520ms ± 1%    ~     (p=0.056 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=0.00/numRangeKeys=1-24       570ms ± 0%   559ms ± 0%  -1.98%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=0.00/numRangeKeys=100-24     624ms ± 1%   618ms ± 0%  -0.96%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=50.00/numRangeKeys=0-24      1.03s ± 0%   1.02s ± 0%  -0.56%  (p=0.016 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=50.00/numRangeKeys=1-24      1.21s ± 0%   1.21s ± 0%    ~     (p=0.056 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=50.00/numRangeKeys=100-24    1.71s ± 0%   1.70s ± 1%    ~     (p=0.056 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=75.00/numRangeKeys=0-24      898ms ± 0%   906ms ± 0%  +0.86%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=75.00/numRangeKeys=1-24      1.08s ± 1%   1.07s ± 1%    ~     (p=0.421 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=75.00/numRangeKeys=100-24    1.44s ± 0%   1.44s ± 0%    ~     (p=0.841 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=95.00/numRangeKeys=0-24      190ms ± 1%   189ms ± 1%    ~     (p=0.056 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=95.00/numRangeKeys=1-24      215ms ± 0%   215ms ± 1%    ~     (p=0.421 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=95.00/numRangeKeys=100-24    325ms ± 1%   324ms ± 1%    ~     (p=0.548 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=99.00/numRangeKeys=0-24      158ms ± 1%   158ms ± 1%    ~     (p=0.690 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=99.00/numRangeKeys=1-24      181ms ± 0%   182ms ± 0%  +0.73%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=true/perc=99.00/numRangeKeys=100-24    294ms ± 0%   293ms ± 1%    ~     (p=0.421 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=0-24      782ms ± 0%   788ms ± 1%  +0.81%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=1-24      901ms ± 0%   906ms ± 0%  +0.55%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=100-24    918ms ± 1%   920ms ± 0%    ~     (p=0.310 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=0-24     663ms ± 1%   671ms ± 0%  +1.21%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=1-24     793ms ± 0%   799ms ± 0%  +0.68%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=100-24   803ms ± 0%   813ms ± 1%  +1.25%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=75.00/numRangeKeys=0-24     594ms ± 0%   601ms ± 0%  +1.28%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=75.00/numRangeKeys=1-24     722ms ± 0%   730ms ± 1%  +1.17%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=75.00/numRangeKeys=100-24   733ms ± 0%   742ms ± 1%  +1.20%  (p=0.008 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=0-24     172ms ± 0%   170ms ± 1%  -0.70%  (p=0.032 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=1-24     194ms ± 0%   194ms ± 1%    ~     (p=0.421 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=100-24   303ms ± 0%   303ms ± 1%    ~     (p=0.841 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=99.00/numRangeKeys=0-24     153ms ± 0%   154ms ± 0%  +0.59%  (p=0.016 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=99.00/numRangeKeys=1-24     177ms ± 1%   177ms ± 1%    ~     (p=0.690 n=5+5)
CatchUpScan/mixed-case/withDiff=false/perc=99.00/numRangeKeys=100-24   288ms ± 1%   288ms ± 0%    ~     (p=0.841 n=5+5)
```

Release justification: bug fixes and low-risk updates to new functionality

Release note: None
@erikgrinaker
Copy link
Contributor Author

bors r=tbg

@craig
Copy link
Contributor

craig bot commented Aug 22, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 22, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 22, 2022

Build succeeded:

@craig craig bot merged commit 76f5b3a into cockroachdb:master Aug 22, 2022
@erikgrinaker erikgrinaker deleted the catchup-scan-rangekeychanged branch August 23, 2022 08:45
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