mvcc: reuse iter for get and put in cput by dt · Pull Request #4308 · cockroachdb/cockroach

dt · 2016-02-10T22:47:32Z

Not sure if we actually want to merge this, but making a PR for ease of discussion.

name                              old time/op    new time/op    delta
    MVCCConditionalPutCreate10-8        5.87µs ± 2%    5.63µs ± 5%   -4.03%   (p=0.000 n=10+9)
    MVCCConditionalPutCreate100-8       6.03µs ± 2%    5.37µs ± 9%  -10.93%  (p=0.000 n=10+10)
    MVCCConditionalPutCreate1000-8      8.29µs ±13%    7.80µs ±12%     ~     (p=0.315 n=10+10)
    MVCCConditionalPutCreate10000-8     29.6µs ± 7%    30.2µs ± 6%     ~     (p=0.353 n=10+10)
    MVCCConditionalPutReplace10-8       8.73µs ± 4%    7.04µs ± 7%  -19.36%  (p=0.000 n=10+10)
    MVCCConditionalPutReplace100-8      9.02µs ± 2%    7.34µs ± 7%  -18.58%   (p=0.000 n=9+10)
    MVCCConditionalPutReplace1000-8     15.0µs ± 6%    13.6µs ± 6%   -9.48%   (p=0.000 n=9+10)
    MVCCConditionalPutReplace10000-8    56.6µs ± 3%    54.6µs ± 6%     ~      (p=0.055 n=8+10)

    name                              old speed      new speed      delta
    MVCCConditionalPutCreate10-8      1.70MB/s ± 1%  1.78MB/s ± 5%   +4.57%    (p=0.000 n=9+9)
    MVCCConditionalPutCreate100-8     16.6MB/s ± 2%  18.7MB/s ± 9%  +12.66%  (p=0.000 n=10+10)
    MVCCConditionalPutCreate1000-8     121MB/s ±11%   129MB/s ±13%     ~     (p=0.315 n=10+10)
    MVCCConditionalPutCreate10000-8    338MB/s ± 7%   332MB/s ± 6%     ~     (p=0.353 n=10+10)
    MVCCConditionalPutReplace10-8     1.15MB/s ± 4%  1.42MB/s ± 6%  +24.08%  (p=0.000 n=10+10)
    MVCCConditionalPutReplace100-8    11.1MB/s ± 2%  13.6MB/s ± 7%  +23.02%   (p=0.000 n=9+10)
    MVCCConditionalPutReplace1000-8   66.7MB/s ± 6%  73.7MB/s ± 6%  +10.49%   (p=0.000 n=9+10)
    MVCCConditionalPutReplace10000-8   177MB/s ± 3%   183MB/s ± 6%     ~      (p=0.055 n=8+10)

    name                              old alloc/op   new alloc/op   delta
    MVCCConditionalPutCreate10-8          128B ± 0%       64B ± 0%  -50.00%  (p=0.000 n=10+10)
    MVCCConditionalPutCreate100-8         128B ± 0%       64B ± 0%  -50.00%  (p=0.000 n=10+10)
    MVCCConditionalPutCreate1000-8        128B ± 0%       64B ± 0%  -50.00%  (p=0.000 n=10+10)
    MVCCConditionalPutCreate10000-8       128B ± 0%       64B ± 0%  -50.00%  (p=0.000 n=10+10)
    MVCCConditionalPutReplace10-8         192B ± 0%      128B ± 0%  -33.33%  (p=0.000 n=10+10)
    MVCCConditionalPutReplace100-8        288B ± 0%      224B ± 0%  -22.22%  (p=0.000 n=10+10)
    MVCCConditionalPutReplace1000-8     1.20kB ± 0%    1.14kB ± 0%   -5.41%  (p=0.000 n=10+10)
    MVCCConditionalPutReplace10000-8    10.7kB ± 0%    10.6kB ± 0%   -0.60%  (p=0.000 n=10+10)

    name                              old allocs/op  new allocs/op  delta
    MVCCConditionalPutCreate10-8          2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.000 n=10+10)
    MVCCConditionalPutCreate100-8         2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.000 n=10+10)
    MVCCConditionalPutCreate1000-8        2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.000 n=10+10)
    MVCCConditionalPutCreate10000-8       2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.000 n=10+10)
    MVCCConditionalPutReplace10-8         4.00 ± 0%      3.00 ± 0%  -25.00%  (p=0.000 n=10+10)
    MVCCConditionalPutReplace100-8        4.00 ± 0%      3.00 ± 0%  -25.00%  (p=0.000 n=10+10)
    MVCCConditionalPutReplace1000-8       4.00 ± 0%      3.00 ± 0%  -25.00%  (p=0.000 n=10+10)
    MVCCConditionalPutReplace10000-8      4.00 ± 0%      3.00 ± 0%  -25.00%  (p=0.000 n=10+10)

The difference doesn't affect actual query performance much though:

name                  old time/op    new time/op    delta
Insert10_Cockroach-8     849µs ± 1%     861µs ± 3%    ~            (p=0.065 n=9+10)
Update10_Cockroach-8    5.84ms ± 5%    5.79ms ± 1%    ~           (p=0.529 n=10+10)
Delete10_Cockroach-8    2.08ms ± 1%    2.11ms ± 2%  +1.69%        (p=0.001 n=10+10)
Scan10_Cockroach-8       303µs ± 3%     302µs ± 0%    ~            (p=0.460 n=10+8)

name                  old alloc/op   new alloc/op   delta
Insert10_Cockroach-8    81.9kB ± 0%    81.2kB ± 0%  -0.78%        (p=0.000 n=10+10)
Update10_Cockroach-8     278kB ± 0%     278kB ± 0%    ~           (p=0.579 n=10+10)
Delete10_Cockroach-8     139kB ± 0%     139kB ± 0%    ~           (p=0.105 n=10+10)
Scan10_Cockroach-8      17.1kB ± 0%    17.0kB ± 0%    ~           (p=0.592 n=10+10)

name                  old allocs/op  new allocs/op  delta
Insert10_Cockroach-8       859 ± 0%       849 ± 0%  -1.16%        (p=0.000 n=10+10)
Update10_Cockroach-8     5.41k ± 0%     5.41k ± 0%    ~     (all samples are equal)
Delete10_Cockroach-8     1.63k ± 0%     1.63k ± 0%    ~           (p=1.000 n=10+10)
Scan10_Cockroach-8         311 ± 0%       311 ± 0%    ~           (p=0.650 n=10+10)

tamird · 2016-02-10T22:54:55Z

LGTM. Nice.

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

storage/engine/mvcc.go, line 786 [r1] (raw file):
remove

storage/engine/mvcc.go, line 789 [r1] (raw file):
remove (or add a similar newline in MVCCGet

Comments from the review on Reviewable.io

dt · 2016-02-10T23:05:26Z

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.

storage/engine/mvcc.go, line 786 [r1] (raw file):
Done.

storage/engine/mvcc.go, line 789 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

tbg · 2016-02-10T23:05:46Z

LGTM.

dt · 2016-02-10T23:08:06Z

I'm still not sure if it is worth merging -- it doesn't seems to move the SQL benchmarks appreciably.

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

tbg · 2016-02-10T23:11:57Z

The performance doesn't really indicate that we must merge this, but it seems "pretty" to not run through more than one iterator for one "operation". @petermattis?

tamird · 2016-02-10T23:12:57Z

Yeah, this is a nice cleanup as far as I'm concerned.

tamird · 2016-02-10T23:13:27Z

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

petermattis · 2016-02-11T00:49:28Z

LGTM. I was hoping this was going to have a bigger impact. Regardless, I think this is worth merging.

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

storage/engine/mvcc.go, line 544 [r2] (raw file):
I don't think the engine parameter is used anymore. For a bit of additional consistency with other functions in this file (e.g. mvccGetInternal), I'd make iter the first parameter.

storage/engine/mvcc.go, line 794 [r2] (raw file):
Let's make iter the second parameter after engine.

storage/engine/mvcc.go, line 1019 [r2] (raw file):
One additional change to be made: when mvccPutUsingIter is called here, the iter might already be pointing at the correct key. This will show up in the mvccGetMetadata call done at the beginning of mvccPutInternal. I think we can do a quick check at the beginning of mvccGetMetadata and avoid performing a call to seek.

Comments from the review on Reviewable.io

dt · 2016-02-11T04:25:38Z

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions.

storage/engine/mvcc.go, line 544 [r2] (raw file):
Done.

storage/engine/mvcc.go, line 794 [r2] (raw file):
Done.

storage/engine/mvcc.go, line 1019 [r2] (raw file):
Hm, my first attempt at that didn't quite work (compiles but isn't correct). Either way, I'm inclined to split do that in a followup.

Comments from the review on Reviewable.io

petermattis · 2016-02-11T14:47:43Z

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.

storage/engine/mvcc.go, line 1019 [r2] (raw file):
A follow-on PR is fine by me.

Comments from the review on Reviewable.io

petermattis · 2016-02-11T14:47:49Z

LGTM

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

``` name old time/op new time/op delta MVCCConditionalPutCreate10-8 5.87µs ± 2% 5.63µs ± 5% -4.03% (p=0.000 n=10+9) MVCCConditionalPutCreate100-8 6.03µs ± 2% 5.37µs ± 9% -10.93% (p=0.000 n=10+10) MVCCConditionalPutCreate1000-8 8.29µs ±13% 7.80µs ±12% ~ (p=0.315 n=10+10) MVCCConditionalPutCreate10000-8 29.6µs ± 7% 30.2µs ± 6% ~ (p=0.353 n=10+10) MVCCConditionalPutReplace10-8 8.73µs ± 4% 7.04µs ± 7% -19.36% (p=0.000 n=10+10) MVCCConditionalPutReplace100-8 9.02µs ± 2% 7.34µs ± 7% -18.58% (p=0.000 n=9+10) MVCCConditionalPutReplace1000-8 15.0µs ± 6% 13.6µs ± 6% -9.48% (p=0.000 n=9+10) MVCCConditionalPutReplace10000-8 56.6µs ± 3% 54.6µs ± 6% ~ (p=0.055 n=8+10) name old speed new speed delta MVCCConditionalPutCreate10-8 1.70MB/s ± 1% 1.78MB/s ± 5% +4.57% (p=0.000 n=9+9) MVCCConditionalPutCreate100-8 16.6MB/s ± 2% 18.7MB/s ± 9% +12.66% (p=0.000 n=10+10) MVCCConditionalPutCreate1000-8 121MB/s ±11% 129MB/s ±13% ~ (p=0.315 n=10+10) MVCCConditionalPutCreate10000-8 338MB/s ± 7% 332MB/s ± 6% ~ (p=0.353 n=10+10) MVCCConditionalPutReplace10-8 1.15MB/s ± 4% 1.42MB/s ± 6% +24.08% (p=0.000 n=10+10) MVCCConditionalPutReplace100-8 11.1MB/s ± 2% 13.6MB/s ± 7% +23.02% (p=0.000 n=9+10) MVCCConditionalPutReplace1000-8 66.7MB/s ± 6% 73.7MB/s ± 6% +10.49% (p=0.000 n=9+10) MVCCConditionalPutReplace10000-8 177MB/s ± 3% 183MB/s ± 6% ~ (p=0.055 n=8+10) name old alloc/op new alloc/op delta MVCCConditionalPutCreate10-8 128B ± 0% 64B ± 0% -50.00% (p=0.000 n=10+10) MVCCConditionalPutCreate100-8 128B ± 0% 64B ± 0% -50.00% (p=0.000 n=10+10) MVCCConditionalPutCreate1000-8 128B ± 0% 64B ± 0% -50.00% (p=0.000 n=10+10) MVCCConditionalPutCreate10000-8 128B ± 0% 64B ± 0% -50.00% (p=0.000 n=10+10) MVCCConditionalPutReplace10-8 192B ± 0% 128B ± 0% -33.33% (p=0.000 n=10+10) MVCCConditionalPutReplace100-8 288B ± 0% 224B ± 0% -22.22% (p=0.000 n=10+10) MVCCConditionalPutReplace1000-8 1.20kB ± 0% 1.14kB ± 0% -5.41% (p=0.000 n=10+10) MVCCConditionalPutReplace10000-8 10.7kB ± 0% 10.6kB ± 0% -0.60% (p=0.000 n=10+10) name old allocs/op new allocs/op delta MVCCConditionalPutCreate10-8 2.00 ± 0% 1.00 ± 0% -50.00% (p=0.000 n=10+10) MVCCConditionalPutCreate100-8 2.00 ± 0% 1.00 ± 0% -50.00% (p=0.000 n=10+10) MVCCConditionalPutCreate1000-8 2.00 ± 0% 1.00 ± 0% -50.00% (p=0.000 n=10+10) MVCCConditionalPutCreate10000-8 2.00 ± 0% 1.00 ± 0% -50.00% (p=0.000 n=10+10) MVCCConditionalPutReplace10-8 4.00 ± 0% 3.00 ± 0% -25.00% (p=0.000 n=10+10) MVCCConditionalPutReplace100-8 4.00 ± 0% 3.00 ± 0% -25.00% (p=0.000 n=10+10) MVCCConditionalPutReplace1000-8 4.00 ± 0% 3.00 ± 0% -25.00% (p=0.000 n=10+10) MVCCConditionalPutReplace10000-8 4.00 ± 0% 3.00 ± 0% -25.00% (p=0.000 n=10+10) ``` The difference doesn't affect actual query performance much though: ``` name old time/op new time/op delta Insert10_Cockroach-8 849µs ± 1% 861µs ± 3% ~ (p=0.065 n=9+10) Update10_Cockroach-8 5.84ms ± 5% 5.79ms ± 1% ~ (p=0.529 n=10+10) Delete10_Cockroach-8 2.08ms ± 1% 2.11ms ± 2% +1.69% (p=0.001 n=10+10) Scan10_Cockroach-8 303µs ± 3% 302µs ± 0% ~ (p=0.460 n=10+8) name old alloc/op new alloc/op delta Insert10_Cockroach-8 81.9kB ± 0% 81.2kB ± 0% -0.78% (p=0.000 n=10+10) Update10_Cockroach-8 278kB ± 0% 278kB ± 0% ~ (p=0.579 n=10+10) Delete10_Cockroach-8 139kB ± 0% 139kB ± 0% ~ (p=0.105 n=10+10) Scan10_Cockroach-8 17.1kB ± 0% 17.0kB ± 0% ~ (p=0.592 n=10+10) name old allocs/op new allocs/op delta Insert10_Cockroach-8 859 ± 0% 849 ± 0% -1.16% (p=0.000 n=10+10) Update10_Cockroach-8 5.41k ± 0% 5.41k ± 0% ~ (all samples are equal) Delete10_Cockroach-8 1.63k ± 0% 1.63k ± 0% ~ (p=1.000 n=10+10) Scan10_Cockroach-8 311 ± 0% 311 ± 0% ~ (p=0.650 n=10+10) ```

mvcc: reuse iter for get and put in cput

dt force-pushed the iter branch from 97e7ae5 to 4e2f838 Compare February 10, 2016 23:04

dt force-pushed the iter branch from 4e2f838 to 5355bd4 Compare February 11, 2016 04:19

dt force-pushed the iter branch from 5355bd4 to eb570c7 Compare February 11, 2016 14:11

dt force-pushed the iter branch from eb570c7 to 4541074 Compare February 11, 2016 14:48

dt added a commit that referenced this pull request Feb 11, 2016

Merge pull request #4308 from dt/iter

1afa220

mvcc: reuse iter for get and put in cput

dt merged commit 1afa220 into cockroachdb:master Feb 11, 2016

dt deleted the iter branch February 11, 2016 14:50

dt mentioned this pull request Feb 11, 2016

Fix rocksdb prefix Seek performance. #4319

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mvcc: reuse iter for get and put in cput#4308

mvcc: reuse iter for get and put in cput#4308
dt merged 1 commit intocockroachdb:masterfrom
dt:iter

dt commented Feb 10, 2016

Uh oh!

tamird commented Feb 10, 2016

Uh oh!

dt commented Feb 10, 2016

Uh oh!

tbg commented Feb 10, 2016

Uh oh!

dt commented Feb 10, 2016

Uh oh!

tbg commented Feb 10, 2016

Uh oh!

tamird commented Feb 10, 2016

Uh oh!

tamird commented Feb 10, 2016

Uh oh!

petermattis commented Feb 11, 2016

Uh oh!

dt commented Feb 11, 2016

Uh oh!

petermattis commented Feb 11, 2016

Uh oh!

petermattis commented Feb 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

dt commented Feb 10, 2016

Uh oh!

tamird commented Feb 10, 2016

Uh oh!

dt commented Feb 10, 2016

Uh oh!

tbg commented Feb 10, 2016

Uh oh!

dt commented Feb 10, 2016

Uh oh!

tbg commented Feb 10, 2016

Uh oh!

tamird commented Feb 10, 2016

Uh oh!

tamird commented Feb 10, 2016

Uh oh!

petermattis commented Feb 11, 2016

Uh oh!

dt commented Feb 11, 2016

Uh oh!

petermattis commented Feb 11, 2016

Uh oh!

petermattis commented Feb 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants