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

Move MVCCComputeStats implementation to C++. #3155

Merged
merged 1 commit into from Nov 19, 2015

Conversation

@petermattis
Copy link
Contributor

petermattis commented Nov 17, 2015

This change is a work in progress. I still need to adjust or remove the
usage of ReplicaDataIterator for stats computation in
Replica.splitTrigger.

name                                old time/op    new time/op     delta
MVCCComputeStats1Version8Bytes-8       1.04s ± 2%      0.34s ± 1%   -67.04%  (p=0.000 n=10+10)
MVCCComputeStats1Version32Bytes-8      737ms ± 4%      242ms ± 2%   -67.23%  (p=0.000 n=10+10)
MVCCComputeStats1Version256Bytes-8     211ms ± 3%       86ms ± 4%   -59.38%  (p=0.000 n=10+10)

name                                old speed      new speed       delta
MVCCComputeStats1Version8Bytes-8    64.5MB/s ± 2%  195.7MB/s ± 1%  +203.39%  (p=0.000 n=10+10)
MVCCComputeStats1Version32Bytes-8   91.0MB/s ± 4%  277.8MB/s ± 2%  +205.12%  (p=0.000 n=10+10)
MVCCComputeStats1Version256Bytes-8   319MB/s ± 3%    785MB/s ± 5%  +146.25%  (p=0.000 n=10+10)

Review on Reviewable

@petermattis petermattis force-pushed the pmattis/mvcc-compute-stats-2 branch 2 times, most recently from 60a5101 to aa7ba2c Nov 17, 2015
@petermattis petermattis changed the title [DO NOT MERGE] Move MVCCComputeStats implementation to C++. Move MVCCComputeStats implementation to C++. Nov 18, 2015
@petermattis

This comment has been minimized.

Copy link
Contributor Author

petermattis commented Nov 18, 2015

This is now ready for a look. I wish there was a good way to diff the Go and C++ implementations.

@petermattis petermattis force-pushed the pmattis/mvcc-compute-stats-2 branch from aa7ba2c to 83c3739 Nov 18, 2015
@tamird

This comment has been minimized.

Copy link
Collaborator

tamird commented Nov 18, 2015

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


storage/engine/engine.go, line 76 [r1] (raw file):
this function should take roachpb.Key or mvcc.MVCCKey rather than []byte


storage/engine/mvcc_test.go, line 2548 [r1] (raw file):
would be nice to avoid overwriting err here


storage/engine/rocksdb/db.cc, line 1240 [r1] (raw file):
where did this value come from?


storage/engine/rocksdb/db.cc, line 1260 [r1] (raw file):
s/decoded/decode/


storage/engine/rocksdb/db.cc, line 1269 [r1] (raw file):
nit: can this not duplicate the number 12?


storage/engine/rocksdb/encoding.cc, line 93 [r1] (raw file):
would this read nicer as a loop?


storage/engine/rocksdb/encoding.cc, line 97 [r1] (raw file):
nit: extract local sizeof(*value)


storage/engine/rocksdb/encoding.h, line 34 [r1] (raw file):
did you mean *value? here and below


storage/engine/rocksdb_test.go, line 556 [r1] (raw file):
remove this


storage/replica_data_iter.go, line 70 [r1] (raw file):
how come this could be removed?


Comments from the review on Reviewable.io

@petermattis

This comment has been minimized.

Copy link
Contributor Author

petermattis commented Nov 18, 2015

Review status: all files reviewed at latest revision, 10 unresolved discussions.


storage/engine/rocksdb/db.cc, line 1240 [r1] (raw file):
It's a constant in mvcc.go: mvccVersionTimestampSize.


storage/engine/rocksdb/db.cc, line 1260 [r1] (raw file):
Done.


storage/engine/rocksdb/db.cc, line 1269 [r1] (raw file):
Yeah. Added a FmtStatus function which uses the google::protobuf::StringPrintfutility.


storage/engine/rocksdb/encoding.cc, line 93 [r1] (raw file):
This mirrors the code implementation. A loop is slower.


storage/engine/rocksdb/encoding.cc, line 97 [r1] (raw file):
Done.


storage/engine/rocksdb/encoding.h, line 34 [r1] (raw file):
Yes. Copy/paste error.


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


storage/replica_data_iter.go, line 70 [r1] (raw file):
ri.Seek() already does an advance(). This was a no-op.


Comments from the review on Reviewable.io

@petermattis

This comment has been minimized.

Copy link
Contributor Author

petermattis commented Nov 18, 2015

Review status: all files reviewed at latest revision, 10 unresolved discussions.


storage/engine/engine.go, line 76 [r1] (raw file):
All of the other methods here take a []byte for keys. Feel free to change this in a subsequent PR.


storage/engine/mvcc_test.go, line 2548 [r1] (raw file):
I don't see why it matters, and mildly irritating to do. Certainly wouldn't improve readability. Note that we were overwriting err in the previous code too.


Comments from the review on Reviewable.io

@tamird

This comment has been minimized.

Copy link
Collaborator

tamird commented Nov 18, 2015

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


storage/engine/rocksdb/db.cc, line 1273 [r2] (raw file):
Did you want to log the key here? I don't feel strongly, but since you're using FmtStatus, might be nice.


Comments from the review on Reviewable.io

@petermattis

This comment has been minimized.

Copy link
Contributor Author

petermattis commented Nov 18, 2015

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


storage/engine/rocksdb/db.cc, line 1273 [r2] (raw file):
This is using c-printf formatting which is much weaker than Go fmt. Not easy to do (unless I've forgotten something).


Comments from the review on Reviewable.io

@@ -115,6 +120,15 @@ DBStatus ToDBStatus(const rocksdb::Status& status) {
return ToDBString(status.ToString());
}

DBStatus FmtStatus(const char *fmt, ...) {

This comment has been minimized.

Copy link
@bdarnell

bdarnell Nov 19, 2015

Member

Is this function covered in our tests anywhere? I didn't see any cases where we test for a failure of MVCCComputeStats, but I didn't do an exhaustive search. Would be good to have tests to make sure we get the memory management right here.

This comment has been minimized.

Copy link
@petermattis

petermattis Nov 19, 2015

Author Contributor

FmtStatus is not explicit tested, but we do have tests that exercise the code path that memory allocated by ToDBString is freed by statusToError. Do you have any thoughts on how to test that the memory was actually freed?

This comment has been minimized.

Copy link
@bdarnell

bdarnell Nov 19, 2015

Member

I'm more concerned about returning a pointer to freed memory than a leak (I don't see any practical way to test for the absence of a leak). I'd just like to see a test where MVCCStats returns an error to make sure we execute FmtStatus at least once.

This comment has been minimized.

Copy link
@petermattis

petermattis Nov 19, 2015

Author Contributor

Added TestMVCCComputeStatsError which writes an mvcc metadata key with garbage for the value.

@bdarnell

This comment has been minimized.

Copy link
Member

bdarnell commented Nov 19, 2015

LGTM

Replaced the Go MVCCComputeStats implementation with a C++
implementation. Had to rejigger the usage of ComputeStats during splits
and merges to properly loop over all of the key ranges for a replica
because the C++ implementation is unable to use the ReplicaDataIterator
directly. Small price to pay for a big speed-up.

name                                old time/op    new time/op     delta
MVCCComputeStats1Version8Bytes-8       1.04s ± 2%      0.34s ± 1%   -67.04%  (p=0.000 n=10+10)
MVCCComputeStats1Version32Bytes-8      737ms ± 4%      242ms ± 2%   -67.23%  (p=0.000 n=10+10)
MVCCComputeStats1Version256Bytes-8     211ms ± 3%       86ms ± 4%   -59.38%  (p=0.000 n=10+10)

name                                old speed      new speed       delta
MVCCComputeStats1Version8Bytes-8    64.5MB/s ± 2%  195.7MB/s ± 1%  +203.39%  (p=0.000 n=10+10)
MVCCComputeStats1Version32Bytes-8   91.0MB/s ± 4%  277.8MB/s ± 2%  +205.12%  (p=0.000 n=10+10)
MVCCComputeStats1Version256Bytes-8   319MB/s ± 3%    785MB/s ± 5%  +146.25%  (p=0.000 n=10+10)
@petermattis petermattis force-pushed the pmattis/mvcc-compute-stats-2 branch from a6e57f7 to 984b878 Nov 19, 2015
petermattis added a commit that referenced this pull request Nov 19, 2015
Move MVCCComputeStats implementation to C++.
@petermattis petermattis merged commit daf96c3 into master Nov 19, 2015
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable Review in progress: 9 of 11 files reviewed, 1 unresolved discussion
Details
ci/circleci Your tests passed on CircleCI!
Details
licence/cla Contributor License Agreement is signed.
Details
@petermattis petermattis deleted the pmattis/mvcc-compute-stats-2 branch Nov 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.