Move MVCCComputeStats implementation to C++. #3155

petermattis · 2015-11-17T17:03:58Z

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)

petermattis · 2015-11-18T02:42:18Z

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

tamird · 2015-11-18T18:39:44Z

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 · 2015-11-18T22:39:01Z

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 · 2015-11-18T22:44:11Z

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 · 2015-11-18T22:48:38Z

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 · 2015-11-18T22:50:38Z

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

bdarnell · 2015-11-19T00:05:29Z

storage/engine/rocksdb/db.cc

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

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


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.

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?

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.

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

bdarnell · 2015-11-19T00:10:17Z

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)

Move MVCCComputeStats implementation to C++.

petermattis force-pushed the pmattis/mvcc-compute-stats-2 branch 2 times, most recently from 60a5101 to aa7ba2c Compare November 18, 2015 02:40

petermattis changed the title ~~[DO NOT MERGE] Move MVCCComputeStats implementation to C++.~~ Move MVCCComputeStats implementation to C++. Nov 18, 2015

petermattis force-pushed the pmattis/mvcc-compute-stats-2 branch from aa7ba2c to 83c3739 Compare November 18, 2015 16:59

bdarnell reviewed Nov 19, 2015
View reviewed changes

petermattis force-pushed the pmattis/mvcc-compute-stats-2 branch from a6e57f7 to 984b878 Compare November 19, 2015 16:39

petermattis added a commit that referenced this pull request Nov 19, 2015

Merge pull request #3155 from cockroachdb/pmattis/mvcc-compute-stats-2

daf96c3

Move MVCCComputeStats implementation to C++.

petermattis merged commit daf96c3 into master Nov 19, 2015

petermattis deleted the pmattis/mvcc-compute-stats-2 branch November 19, 2015 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move MVCCComputeStats implementation to C++. #3155

Move MVCCComputeStats implementation to C++. #3155

petermattis commented Nov 17, 2015

petermattis commented Nov 18, 2015

tamird commented Nov 18, 2015

petermattis commented Nov 18, 2015

petermattis commented Nov 18, 2015

tamird commented Nov 18, 2015

petermattis commented Nov 18, 2015

bdarnell Nov 19, 2015

petermattis Nov 19, 2015

bdarnell Nov 19, 2015

petermattis Nov 19, 2015

bdarnell commented Nov 19, 2015

Move MVCCComputeStats implementation to C++. #3155

Move MVCCComputeStats implementation to C++. #3155

Conversation

petermattis commented Nov 17, 2015

petermattis commented Nov 18, 2015

tamird commented Nov 18, 2015

petermattis commented Nov 18, 2015

petermattis commented Nov 18, 2015

tamird commented Nov 18, 2015

petermattis commented Nov 18, 2015

bdarnell Nov 19, 2015

Choose a reason for hiding this comment

petermattis Nov 19, 2015

Choose a reason for hiding this comment

bdarnell Nov 19, 2015

Choose a reason for hiding this comment

petermattis Nov 19, 2015

Choose a reason for hiding this comment

bdarnell commented Nov 19, 2015