{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/3155","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/3155/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/3155/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/3155/events","html_url":"https://github.com/cockroachdb/cockroach/pull/3155","id":117400158,"number":3155,"title":"Move MVCCComputeStats implementation to C++.","user":{"login":"petermattis","id":6201335,"avatar_url":"https://avatars.githubusercontent.com/u/6201335?v=3","gravatar_id":"","url":"https://api.github.com/users/petermattis","html_url":"https://github.com/petermattis","followers_url":"https://api.github.com/users/petermattis/followers","following_url":"https://api.github.com/users/petermattis/following{/other_user}","gists_url":"https://api.github.com/users/petermattis/gists{/gist_id}","starred_url":"https://api.github.com/users/petermattis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petermattis/subscriptions","organizations_url":"https://api.github.com/users/petermattis/orgs","repos_url":"https://api.github.com/users/petermattis/repos","events_url":"https://api.github.com/users/petermattis/events{/privacy}","received_events_url":"https://api.github.com/users/petermattis/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2015-11-17T17:03:58Z","updated_at":"2015-11-18T18:39:44Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/3155","html_url":"https://github.com/cockroachdb/cockroach/pull/3155","diff_url":"https://github.com/cockroachdb/cockroach/pull/3155.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/3155.patch"},"body":"This change is a work in progress. I still need to adjust or remove the\nusage of ReplicaDataIterator for stats computation in\nReplica.splitTrigger.\n\nname                                old time/op    new time/op     delta\nMVCCComputeStats1Version8Bytes-8       1.04s ± 2%      0.34s ± 1%   -67.04%  (p=0.000 n=10+10)\nMVCCComputeStats1Version32Bytes-8      737ms ± 4%      242ms ± 2%   -67.23%  (p=0.000 n=10+10)\nMVCCComputeStats1Version256Bytes-8     211ms ± 3%       86ms ± 4%   -59.38%  (p=0.000 n=10+10)\n\nname                                old speed      new speed       delta\nMVCCComputeStats1Version8Bytes-8    64.5MB/s ± 2%  195.7MB/s ± 1%  +203.39%  (p=0.000 n=10+10)\nMVCCComputeStats1Version32Bytes-8   91.0MB/s ± 4%  277.8MB/s ± 2%  +205.12%  (p=0.000 n=10+10)\nMVCCComputeStats1Version256Bytes-8   319MB/s ± 3%    785MB/s ± 5%  +146.25%  (p=0.000 n=10+10)\n\n<!-- Reviewable:start -->\n[<img src=\"https://reviewable.io/review_button.png\" height=40 alt=\"Review on Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/3155)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/157814676","html_url":"https://github.com/cockroachdb/cockroach/pull/3155#issuecomment-157814676","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/3155","id":157814676,"user":{"login":"tamird","id":1535036,"avatar_url":"https://avatars.githubusercontent.com/u/1535036?v=3","gravatar_id":"","url":"https://api.github.com/users/tamird","html_url":"https://github.com/tamird","followers_url":"https://api.github.com/users/tamird/followers","following_url":"https://api.github.com/users/tamird/following{/other_user}","gists_url":"https://api.github.com/users/tamird/gists{/gist_id}","starred_url":"https://api.github.com/users/tamird/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tamird/subscriptions","organizations_url":"https://api.github.com/users/tamird/orgs","repos_url":"https://api.github.com/users/tamird/repos","events_url":"https://api.github.com/users/tamird/events{/privacy}","received_events_url":"https://api.github.com/users/tamird/received_events","type":"User","site_admin":false},"created_at":"2015-11-18T18:39:44Z","updated_at":"2015-11-18T18:39:44Z","body":"\n\n\n\nReviewed 11 of 11 files at r1.\nReview status: all files reviewed at latest revision, 10 unresolved discussions.\n\n---\n\n_[storage/engine/engine.go, line 76 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3155#-K3Qnlw8vlZy67x_k6xM:-K3Qnlw9_FI9L41JxKKx:1421308124) ([raw file](https://github.com/cockroachdb/cockroach/blob/83c37394736fc55492dff4136c1265ee1c9795bb/storage/engine/engine.go#L76)):_\nthis function should take `roachpb.Key` or `mvcc.MVCCKey` rather than `[]byte`\n\n---\n\n_[storage/engine/mvcc_test.go, line 2548 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3155#-K3QoH4nLQ-6-ONBthhk:-K3QoH4nLQ-6-ONBthhl:665103079) ([raw file](https://github.com/cockroachdb/cockroach/blob/83c37394736fc55492dff4136c1265ee1c9795bb/storage/engine/mvcc_test.go#L2548)):_\nwould be nice to avoid overwriting `err` here\n\n---\n\n_[storage/engine/rocksdb/db.cc, line 1240 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3155#-K3Qs78c2pFp_bTStasX:-K3Qs78c2pFp_bTStasY:1699536011) ([raw file](https://github.com/cockroachdb/cockroach/blob/83c37394736fc55492dff4136c1265ee1c9795bb/storage/engine/rocksdb/db.cc#L1240)):_\nwhere did this value come from?\n\n---\n\n_[storage/engine/rocksdb/db.cc, line 1260 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3155#-K3QscGiy1m9KqpF6_Td:-K3QscGiy1m9KqpF6_Te:548825895) ([raw file](https://github.com/cockroachdb/cockroach/blob/83c37394736fc55492dff4136c1265ee1c9795bb/storage/engine/rocksdb/db.cc#L1260)):_\ns/decoded/decode/\n\n---\n\n_[storage/engine/rocksdb/db.cc, line 1269 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3155#-K3QsnhVNgRaVTJ4i9hU:-K3QsnhVNgRaVTJ4i9hV:-1908949648) ([raw file](https://github.com/cockroachdb/cockroach/blob/83c37394736fc55492dff4136c1265ee1c9795bb/storage/engine/rocksdb/db.cc#L1269)):_\nnit: can this not duplicate the number 12?\n\n---\n\n_[storage/engine/rocksdb/encoding.cc, line 93 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3155#-K3Qrl7PWOT0Bq6X0-GV:-K3Qrl7PWOT0Bq6X0-GW:141176433) ([raw file](https://github.com/cockroachdb/cockroach/blob/83c37394736fc55492dff4136c1265ee1c9795bb/storage/engine/rocksdb/encoding.cc#L93)):_\nwould this read nicer as a loop?\n\n---\n\n_[storage/engine/rocksdb/encoding.cc, line 97 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3155#-K3QrrPOWDY2k3Gm_Ihh:-K3QrrPOWDY2k3Gm_Ihi:-211174788) ([raw file](https://github.com/cockroachdb/cockroach/blob/83c37394736fc55492dff4136c1265ee1c9795bb/storage/engine/rocksdb/encoding.cc#L97)):_\nnit: extract local `sizeof(*value)`\n\n---\n\n_[storage/engine/rocksdb/encoding.h, line 34 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3155#-K3QrIpaCMyjPZEJrWuH:-K3QrIpaCMyjPZEJrWuI:-1506096058) ([raw file](https://github.com/cockroachdb/cockroach/blob/83c37394736fc55492dff4136c1265ee1c9795bb/storage/engine/rocksdb/encoding.h#L34)):_\ndid you mean `*value`? here and below\n\n---\n\n_[storage/engine/rocksdb_test.go, line 556 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3155#-K3Qqu_VLi4FOw4PH4-h:-K3Qqu_VLi4FOw4PH4-i:-1542324587) ([raw file](https://github.com/cockroachdb/cockroach/blob/83c37394736fc55492dff4136c1265ee1c9795bb/storage/engine/rocksdb_test.go#L556)):_\nremove this\n\n---\n\n_[storage/replica_data_iter.go, line 70 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/3155#-K3QrA-LXzj7aVGvLFVh:-K3QrA-LXzj7aVGvLFVi:-110463902) ([raw file](https://github.com/cockroachdb/cockroach/blob/623d09252c5e3238b8951a6d09e7dd25905f77f1/storage/replica_data_iter.go#L70)):_\nhow come this could be removed?\n\n---\n\n\n_Comments from the [review on Reviewable.io](https://reviewable.io:443/reviews/cockroachdb/cockroach/3155)_\n<!-- Sent from Reviewable.io -->\n"}}