storage: remove key comparison in MVCC stats computations#85580
storage: remove key comparison in MVCC stats computations#85580craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
f05082c to
573cf07
Compare
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewed 4 of 22 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @jbowens)
pkg/kv/kvserver/replica_consistency.go line 708 at r1 (raw file):
// TODO(sumeer): When we have replicated locks other than exclusive locks, // we will probably not have any interleaved intents so we could stop // using MVCCKeyAndIntentsIterKind and consider all locks here.
I can't remember what this TODO is trying to say. Is it merely saying that we can avoid using MVCCKeyAndIntentsIterKind and iterate separately over the lock table and the MVCC keys? If yes, I don't see the connection with "locks other than exclusive locks".
pkg/kv/kvserver/rditer/stats.go line 24 at r1 (raw file):
// TODO(sumeer): When we have replicated locks other than exclusive locks, // we will probably not have any interleaved intents so we could stop // using MVCCKeyAndIntentsIterKind and consider all locks here.
this new location for this TODO doesn't seem right -- we are not creating an MVCCIterator here. We do that in ComputeStatsWithVisitors.
573cf07 to
bb17474
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica_consistency.go line 708 at r1 (raw file):
Previously, sumeerbhola wrote…
I can't remember what this TODO is trying to say. Is it merely saying that we can avoid using MVCCKeyAndIntentsIterKind and iterate separately over the lock table and the MVCC keys? If yes, I don't see the connection with "locks other than exclusive locks".
Glad I'm not the only one confused by this. :) That's my interpretation too, but I removed the comment -- the vast majority of data we scan across is going to be in the user span, so I don't think we're saving much by dropping the intentInterleavingIter for the few spans where we can avoid it.
pkg/kv/kvserver/rditer/stats.go line 24 at r1 (raw file):
Previously, sumeerbhola wrote…
this new location for this TODO doesn't seem right -- we are not creating an MVCCIterator here. We do that in
ComputeStatsWithVisitors.
Yeah, but ComputeStatsWithVisitors can't make that determination, it needs to be plumbed down from the call site so I put it at the relevant call site instead.
jbowens
left a comment
There was a problem hiding this comment.
Reviewed 12 of 22 files at r1, 12 of 12 files at r2, all commit messages.
Reviewable status:
pkg/storage/engine.go line 271 at r2 (raw file):
// could be better solved by checking the iterator bounds in NewMVCCIterator // and requiring callers to set them appropriately. ComputeStats(start, end roachpb.Key, nowNanos int64) (enginepb.MVCCStats, error)
Glad to see this gone
bb17474 to
d562b22
Compare
This patch restructures MVCC stats computations to use an MVCC iterator with appropriate bounds. This allows omitting a key comparison in the hot path, yielding a ~10% performance improvement. The new stats functions are: * `ComputeStats`: calculates stats for a `Reader` key span. * `ComputeStatsWithVisitors`: calculates stats for a `Reader` key span, calling the given visitor callbacks for every point/range key. * `ComputeStatsForIter`: calculates stats for a given `MVCCIterator`, by scanning it until exhausted. It also removes the `MVCCIterator.ComputeStats` method, which had no business being part of the interface. ``` name old time/op new time/op delta MVCCComputeStats_Pebble/valueSize=64-24 130ms ± 1% 119ms ± 1% -8.77% (p=0.000 n=10+10) name old speed new speed delta MVCCComputeStats_Pebble/valueSize=64-24 516MB/s ± 1% 565MB/s ± 1% +9.61% (p=0.000 n=10+10) ``` Release note: None
d562b22 to
7f61dfd
Compare
|
Looks like I won the flake jackpot, with all 3 builds flaking with different failures. Assuming this is coincidental and YOLOing it. bors r=jbowens |
|
👎 Rejected by code reviews |
|
bors r=jbowens |
|
Build succeeded: |
This patch restructures MVCC stats computations to use an MVCC iterator
with appropriate bounds. This allows omitting a key comparison in the
hot path, yielding a ~10% performance improvement. The new stats
functions are:
ComputeStats: calculates stats for aReaderkey span.ComputeStatsWithVisitors: calculates stats for aReaderkey span,calling the given visitor callbacks for every point/range key.
ComputeStatsForIter: calculates stats for a givenMVCCIterator, byscanning it until exhausted.
It also removes the
MVCCIterator.ComputeStatsmethod, which had nobusiness being part of the interface.
Resolves #41899.
Touches #84544.
Release note: None