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

[crl-release-21.1] *: Skip first iteration of read sample #1101

Merged

Conversation

itsbilal
Copy link
Contributor

Backport of #1098.


Currently, we always sample on the first read in a newly-opened
iterator, for determining read compactions. This is because the
zero value of bytesUntilReadSample
is always less than the bytes read. In cases where iterators are
short-lived, such as in ycsb workloads, this makes a greater proportion
of reads sampled than we'd like them to be, and it also makes any
changes to the sampling rate less effective.

This change updates Iterator to detect and skip the first time
it tries to sample a read on a newly-opened iterator.

Also makes a change to use ints instead of uints when
copying over options, to allow for disabling read sampling
by specifying a negative value for ReadSamplingMultiplier
if necessary.

When this change was tested with cockroachdb master, this
performance improvement was had on roachtests compared to
a clean master:

name            old ops/sec  new ops/sec  delta
ycsb/A/nodes=3   17.6k ± 9%   20.1k ± 5%  +14.19%  (p=0.008 n=5+5)
ycsb/B/nodes=3   33.7k ± 5%   35.6k ± 5%   +5.61%  (p=0.032 n=5+5)
ycsb/C/nodes=3   45.2k ± 3%   44.6k ± 3%     ~     (p=0.421 n=5+5)

name            old p50      new p50      delta
ycsb/A/nodes=3    3.56 ± 7%    3.14 ± 5%  -11.80%  (p=0.024 n=5+5)
ycsb/B/nodes=3    3.00 ± 0%    2.84 ± 8%     ~     (p=0.095 n=4+5)
ycsb/C/nodes=3    2.72 ± 4%    2.72 ± 4%     ~     (p=1.000 n=5+5)

name            old p95      new p95      delta
ycsb/A/nodes=3    17.1 ±11%    14.8 ± 4%  -13.35%  (p=0.024 n=5+5)
ycsb/B/nodes=3    13.6 ± 0%    13.0 ± 7%     ~     (p=0.095 n=4+5)
ycsb/C/nodes=3    8.90 ± 0%    8.90 ± 0%     ~     (all equal)

name            old p99      new p99      delta
ycsb/A/nodes=3    32.9 ±27%    27.5 ±18%  -16.52%  (p=0.048 n=5+5)
ycsb/B/nodes=3    25.8 ± 7%    24.3 ±16%     ~     (p=0.302 n=5+5)
ycsb/C/nodes=3    13.6 ± 4%    14.0 ± 3%     ~     (p=0.365 n=5+5)

Currently, we always sample on the first read in a newly-opened
iterator, for determining read compactions. This is because the
zero value of `bytesUntilReadSample`
is always less than the bytes read. In cases where iterators are
short-lived, such as in ycsb workloads, this makes a greater proportion
of reads sampled than we'd like them to be, and it also makes any
changes to the sampling rate less effective.

This change updates Iterator to detect and skip the first time
it tries to sample a read on a newly-opened iterator.

Also makes a change to use `ints` instead of `uints` when
copying over options, to allow for disabling read sampling
by specifying a negative value for ReadSamplingMultiplier
if necessary.

When this change was tested with cockroachdb master, this
performance improvement was had on roachtests compared to
a clean master:

```
name            old ops/sec  new ops/sec  delta
ycsb/A/nodes=3   17.6k ± 9%   20.1k ± 5%  +14.19%  (p=0.008 n=5+5)
ycsb/B/nodes=3   33.7k ± 5%   35.6k ± 5%   +5.61%  (p=0.032 n=5+5)
ycsb/C/nodes=3   45.2k ± 3%   44.6k ± 3%     ~     (p=0.421 n=5+5)

name            old p50      new p50      delta
ycsb/A/nodes=3    3.56 ± 7%    3.14 ± 5%  -11.80%  (p=0.024 n=5+5)
ycsb/B/nodes=3    3.00 ± 0%    2.84 ± 8%     ~     (p=0.095 n=4+5)
ycsb/C/nodes=3    2.72 ± 4%    2.72 ± 4%     ~     (p=1.000 n=5+5)

name            old p95      new p95      delta
ycsb/A/nodes=3    17.1 ±11%    14.8 ± 4%  -13.35%  (p=0.024 n=5+5)
ycsb/B/nodes=3    13.6 ± 0%    13.0 ± 7%     ~     (p=0.095 n=4+5)
ycsb/C/nodes=3    8.90 ± 0%    8.90 ± 0%     ~     (all equal)

name            old p99      new p99      delta
ycsb/A/nodes=3    32.9 ±27%    27.5 ±18%  -16.52%  (p=0.048 n=5+5)
ycsb/B/nodes=3    25.8 ± 7%    24.3 ±16%     ~     (p=0.302 n=5+5)
ycsb/C/nodes=3    13.6 ± 4%    14.0 ± 3%     ~     (p=0.365 n=5+5)
```
@itsbilal itsbilal requested review from jbowens and sumeerbhola March 26, 2021 16:15
@itsbilal itsbilal self-assigned this Mar 26, 2021
@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.

oh I see, you already put this up :)

:LGTM:

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @sumeerbhola)

@itsbilal
Copy link
Contributor Author

Hah yep, TFTR!

@itsbilal itsbilal merged commit b49f2ee into cockroachdb:crl-release-21.1 Mar 26, 2021
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