Skip to content

Add 1PC optimization to delete/update.#4384

Merged
petermattis merged 1 commit intocockroachdb:masterfrom
petermattis:pmattis/delete-update-auto-commit
Feb 15, 2016
Merged

Add 1PC optimization to delete/update.#4384
petermattis merged 1 commit intocockroachdb:masterfrom
petermattis:pmattis/delete-update-auto-commit

Conversation

@petermattis
Copy link
Copy Markdown
Collaborator

@petermattis petermattis commented Feb 15, 2016

The changes in the Update10 and Update100 benchmarks are just noise as
they aren't able to take advantage of the optimization due to using an
explicit transaction. This does highlight that allowing users to specify
that they want the operations within a transaction to be batched could
give a significant speedup.

I need to figure out why Delete100 got slower. That result is
repeatable, but confusing.

name                   old time/op    new time/op    delta
Update1_Cockroach-8       626µs ± 2%     462µs ± 1%  -26.20%        (p=0.000 n=10+10)
Update10_Cockroach-8     3.70ms ± 1%    3.63ms ± 1%   -1.81%         (p=0.000 n=9+10)
Update100_Cockroach-8    32.9ms ± 1%    32.3ms ± 1%   -1.89%        (p=0.000 n=10+10)
Delete1_Cockroach-8       619µs ± 1%     466µs ± 1%  -24.76%        (p=0.000 n=10+10)
Delete10_Cockroach-8     1.26ms ± 2%    1.16ms ± 1%   -8.19%         (p=0.000 n=10+9)
Delete100_Cockroach-8    7.08ms ± 1%    7.50ms ± 1%   +5.89%        (p=0.000 n=10+10)

name                   old allocs/op  new allocs/op  delta
Update1_Cockroach-8         651 ± 0%       488 ± 0%  -25.04%         (p=0.000 n=10+9)
Update10_Cockroach-8      4.47k ± 0%     4.47k ± 0%     ~     (all samples are equal)
Update100_Cockroach-8     42.5k ± 0%     42.5k ± 0%     ~           (p=0.669 n=10+10)
Delete1_Cockroach-8         597 ± 0%       452 ± 0%  -24.29%        (p=0.000 n=10+10)
Delete10_Cockroach-8      1.37k ± 0%     1.20k ± 0%  -12.33%        (p=0.000 n=10+10)
Delete100_Cockroach-8     8.54k ± 0%     8.19k ± 0%   -4.09%         (p=0.000 n=7+10)

Review on Reviewable

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Feb 15, 2016

LGTM

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

sql/bench_test.go, line 243 [r1] (raw file):
Couldn't you parse everything and batch whatever's between a BEGIN and a COMMIT?

Comments from the review on Reviewable.io

@petermattis
Copy link
Copy Markdown
Collaborator Author

petermattis commented Feb 15, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.

sql/bench_test.go, line 243 [r1] (raw file):
a) Getting the error handling for the correct would be tricky and b) We actually need something more than "batching". We'd want to buffer up all of the writes while still performing the reads needed by each update.

Comments from the review on Reviewable.io

@petermattis
Copy link
Copy Markdown
Collaborator Author

petermattis commented Feb 15, 2016

@tschottdorf Any thoughts about that Delete100 slow down? The diff between the two perf numbers amounts to the following code:

        pErr = p.txn.Run(b)
        if pErr == nil {
            pErr = p.txn.Commit()
        }

vs

        pErr = p.txn.CommitInBatch(b)

Surprisingly, the separate Run and Commit is the faster case for Delete100. A cpu profile shows that using CommitInBatch we spend more time in RocksDB calls, though I have yet to determine why.

@petermattis
Copy link
Copy Markdown
Collaborator Author

petermattis commented Feb 15, 2016

I'm going to merge this and file an issue to track down what caused the Delete100 slow down.

@petermattis petermattis mentioned this pull request Feb 15, 2016
Closed
petermattis added a commit that referenced this pull request Feb 15, 2016
@petermattis
Merge pull request #4384 from petermattis/pmattis/delete-update-auto-…
174c365 
…commit

Add 1PC optimization to delete/update.
@petermattis petermattis merged commit 174c365 into cockroachdb:master Feb 15, 2016
@petermattis petermattis deleted the pmattis/delete-update-auto-commit branch February 15, 2016 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@petermattis @tamird