kv/tscache: propagate synthetic timestamps through timestamp cache #57811
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
irfansharif
approved these changes
Dec 11, 2020
There was a problem hiding this comment.
Bit 🐶 🔍 about the existing interval skl implementation, which probably renders my reading of TestIntervalSklRotateWithSyntheticTimestamps as useless, but re: #57683,
Reviewed 1 of 1 files at r1, 3 of 3 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/kv/kvserver/tscache/cache.go, line 117 at r2 (raw file):
// This ratcheting policy is shared across all Cache implementations, even if // they do not use this function directly. func ratchetValue(old, new cacheValue) (cacheValue, bool) {
[nit, not this but] Name this return type.
pkg/kv/kvserver/tscache/cache.go, line 127 at r2 (raw file):
// Equal times. const syn = hlc.TimestampFlag_SYNTHETIC
aside: seems worthwhile exporting a nicer looking symbol from the hlc package.
pkg/kv/kvserver/tscache/interval_skl.go, line 806 at r2 (raw file):
func (p *sklPage) ratchetMaxTimestamp(ts hlc.Timestamp) { new := makeRatchetingTime(ts)
[nit] Here and throughout, avoid "new" if we can.
pkg/kv/kvserver/tscache/interval_skl.go, line 881 at r2 (raw file):
// that if two timestamps have the same ordering but different values for // the flag, the timestamp without the flag has a larger ratchetingTime // value.
Curious why this property is a useful one to have.
pkg/kv/kvserver/tscache/interval_skl.go, line 1231 at r2 (raw file):
binary.BigEndian.PutUint64(b[l:], uint64(val.ts.WallTime)) binary.BigEndian.PutUint32(b[l+8:], uint32(val.ts.Logical)) b[l+12] = byte(val.ts.Flags)
Mind making it clearer where this 12 is coming from? Ditto for decodeValue.
``` name time/op IntervalSklDecodeValue/logical=false,synthetic=false,txnID=false-16 11.7ns ± 2% IntervalSklDecodeValue/logical=false,synthetic=false,txnID=true-16 11.7ns ± 1% IntervalSklDecodeValue/logical=false,synthetic=true,txnID=false-16 11.8ns ± 1% IntervalSklDecodeValue/logical=false,synthetic=true,txnID=true-16 11.7ns ± 0% IntervalSklDecodeValue/logical=true,synthetic=false,txnID=false-16 11.7ns ± 2% IntervalSklDecodeValue/logical=true,synthetic=false,txnID=true-16 11.7ns ± 1% IntervalSklDecodeValue/logical=true,synthetic=true,txnID=false-16 11.7ns ± 1% IntervalSklDecodeValue/logical=true,synthetic=true,txnID=true-16 11.7ns ± 2% IntervalSklEncodeValue/logical=false,synthetic=false,txnID=false-16 6.25ns ± 1% IntervalSklEncodeValue/logical=false,synthetic=false,txnID=true-16 6.25ns ± 1% IntervalSklEncodeValue/logical=false,synthetic=true,txnID=false-16 6.23ns ± 2% IntervalSklEncodeValue/logical=false,synthetic=true,txnID=true-16 6.23ns ± 2% IntervalSklEncodeValue/logical=true,synthetic=false,txnID=false-16 6.26ns ± 3% IntervalSklEncodeValue/logical=true,synthetic=false,txnID=true-16 6.24ns ± 1% IntervalSklEncodeValue/logical=true,synthetic=true,txnID=false-16 6.29ns ± 3% IntervalSklEncodeValue/logical=true,synthetic=true,txnID=true-16 6.23ns ± 2% name alloc/op IntervalSklDecodeValue/logical=false,synthetic=false,txnID=false-16 0.00B IntervalSklDecodeValue/logical=false,synthetic=false,txnID=true-16 0.00B IntervalSklDecodeValue/logical=false,synthetic=true,txnID=false-16 0.00B IntervalSklDecodeValue/logical=false,synthetic=true,txnID=true-16 0.00B IntervalSklDecodeValue/logical=true,synthetic=false,txnID=false-16 0.00B IntervalSklDecodeValue/logical=true,synthetic=false,txnID=true-16 0.00B IntervalSklDecodeValue/logical=true,synthetic=true,txnID=false-16 0.00B IntervalSklDecodeValue/logical=true,synthetic=true,txnID=true-16 0.00B IntervalSklEncodeValue/logical=false,synthetic=false,txnID=false-16 0.00B IntervalSklEncodeValue/logical=false,synthetic=false,txnID=true-16 0.00B IntervalSklEncodeValue/logical=false,synthetic=true,txnID=false-16 0.00B IntervalSklEncodeValue/logical=false,synthetic=true,txnID=true-16 0.00B IntervalSklEncodeValue/logical=true,synthetic=false,txnID=false-16 0.00B IntervalSklEncodeValue/logical=true,synthetic=false,txnID=true-16 0.00B IntervalSklEncodeValue/logical=true,synthetic=true,txnID=false-16 0.00B IntervalSklEncodeValue/logical=true,synthetic=true,txnID=true-16 0.00B name allocs/op IntervalSklDecodeValue/logical=false,synthetic=false,txnID=false-16 0.00 IntervalSklDecodeValue/logical=false,synthetic=false,txnID=true-16 0.00 IntervalSklDecodeValue/logical=false,synthetic=true,txnID=false-16 0.00 IntervalSklDecodeValue/logical=false,synthetic=true,txnID=true-16 0.00 IntervalSklDecodeValue/logical=true,synthetic=false,txnID=false-16 0.00 IntervalSklDecodeValue/logical=true,synthetic=false,txnID=true-16 0.00 IntervalSklDecodeValue/logical=true,synthetic=true,txnID=false-16 0.00 IntervalSklDecodeValue/logical=true,synthetic=true,txnID=true-16 0.00 IntervalSklEncodeValue/logical=false,synthetic=false,txnID=false-16 0.00 IntervalSklEncodeValue/logical=false,synthetic=false,txnID=true-16 0.00 IntervalSklEncodeValue/logical=false,synthetic=true,txnID=false-16 0.00 IntervalSklEncodeValue/logical=false,synthetic=true,txnID=true-16 0.00 IntervalSklEncodeValue/logical=true,synthetic=false,txnID=false-16 0.00 IntervalSklEncodeValue/logical=true,synthetic=false,txnID=true-16 0.00 IntervalSklEncodeValue/logical=true,synthetic=true,txnID=false-16 0.00 IntervalSklEncodeValue/logical=true,synthetic=true,txnID=true-16 0.00 ```
Fixes cockroachdb#57683. Before this change, a read with a synthetic timestamp would not pass the synthetic flag through the timestamp cache to conflicting writes. This could cause issues where a write that was bumped by one of these future timestamps would not notice that its write timestamp was now synthetic. This change fixes this by propagating the synthetic flag through the timestamp cache. We have to play a few tricks to make this efficient around `sklPage.ratchetMaxTimestamp`, which attempts to keeping a running maximum of the timestamps in a given skiplist using atomics. The added complexity here is encapsulated in a new `ratchetingTime` type. The testing around here is actually pretty solid. `TestIntervalSklConcurrency` and `TestIntervalSklConcurrentVsSequential` both perform long series of randomized operations and assert against ratchet inversions. This change adds some randomization around synthetic timestamps to the tests and introduces synthetic timestamps to the ratcheting policy (which wasn't otherwise necessary) to get solid randomized test coverage. This made encoding and decoding timestamp cache values slightly slower, but on the order of nanoseconds. ``` name old time/op new time/op delta IntervalSklDecodeValue/logical=false,synthetic=false,txnID=false-16 11.7ns ± 2% 12.1ns ± 2% +3.52% (p=0.000 n=9+10) IntervalSklDecodeValue/logical=false,synthetic=false,txnID=true-16 11.7ns ± 1% 12.0ns ± 1% +2.66% (p=0.000 n=10+9) IntervalSklDecodeValue/logical=false,synthetic=true,txnID=false-16 11.8ns ± 1% 12.1ns ± 2% +3.27% (p=0.000 n=10+9) IntervalSklDecodeValue/logical=false,synthetic=true,txnID=true-16 11.7ns ± 0% 12.0ns ± 1% +2.82% (p=0.000 n=6+10) IntervalSklDecodeValue/logical=true,synthetic=false,txnID=false-16 11.7ns ± 2% 12.1ns ± 1% +3.08% (p=0.000 n=10+10) IntervalSklDecodeValue/logical=true,synthetic=false,txnID=true-16 11.7ns ± 1% 12.1ns ± 2% +3.59% (p=0.000 n=10+10) IntervalSklDecodeValue/logical=true,synthetic=true,txnID=false-16 11.7ns ± 1% 12.2ns ± 4% +4.11% (p=0.000 n=10+10) IntervalSklDecodeValue/logical=true,synthetic=true,txnID=true-16 11.7ns ± 2% 12.1ns ± 1% +3.42% (p=0.000 n=10+10) IntervalSklEncodeValue/logical=false,synthetic=false,txnID=false-16 6.25ns ± 1% 6.52ns ± 1% +4.38% (p=0.000 n=9+10) IntervalSklEncodeValue/logical=false,synthetic=false,txnID=true-16 6.25ns ± 1% 6.56ns ± 1% +5.00% (p=0.000 n=9+9) IntervalSklEncodeValue/logical=false,synthetic=true,txnID=false-16 6.23ns ± 2% 6.55ns ± 2% +5.07% (p=0.000 n=10+10) IntervalSklEncodeValue/logical=false,synthetic=true,txnID=true-16 6.23ns ± 2% 6.56ns ± 4% +5.28% (p=0.000 n=10+10) IntervalSklEncodeValue/logical=true,synthetic=false,txnID=false-16 6.26ns ± 3% 6.55ns ± 1% +4.66% (p=0.000 n=10+10) IntervalSklEncodeValue/logical=true,synthetic=false,txnID=true-16 6.24ns ± 1% 6.52ns ± 1% +4.53% (p=0.000 n=9+10) IntervalSklEncodeValue/logical=true,synthetic=true,txnID=false-16 6.29ns ± 3% 6.53ns ± 2% +3.71% (p=0.000 n=9+10) IntervalSklEncodeValue/logical=true,synthetic=true,txnID=true-16 6.23ns ± 2% 6.56ns ± 0% +5.26% (p=0.000 n=10+8) name old alloc/op new alloc/op delta IntervalSklDecodeValue/logical=false,synthetic=false,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=false,synthetic=false,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=false,synthetic=true,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=false,synthetic=true,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=false,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=false,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=true,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=true,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=false,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=false,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=true,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=true,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=false,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=false,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=true,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=true,txnID=true-16 0.00B 0.00B ~ (all equal) name old allocs/op new allocs/op delta IntervalSklDecodeValue/logical=false,synthetic=false,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=false,synthetic=false,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=false,synthetic=true,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=false,synthetic=true,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=false,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=false,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=true,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=true,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=false,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=false,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=true,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=true,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=false,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=false,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=true,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=true,txnID=true-16 0.00 0.00 ~ (all equal) ```
These allocations in the benchmark harness were obscuring the behavior of the timestamp cache. Also, remove the unused `parallel` knob in BenchmarkIntervalSklAddAndLookup. ``` name old time/op new time/op delta IntervalSklAddAndLookup/frac_0-16 2.20µs ± 3% 0.61µs ± 5% -72.27% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_1-16 2.07µs ± 3% 0.61µs ± 6% -70.80% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_2-16 1.94µs ± 1% 0.60µs ± 8% -69.02% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_3-16 1.86µs ± 4% 0.59µs ± 9% -68.03% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_4-16 1.76µs ± 5% 0.61µs ± 7% -65.68% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_5-16 1.57µs ± 7% 0.60µs ± 4% -61.64% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_6-16 1.37µs ± 7% 0.60µs ± 6% -56.41% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_7-16 1.18µs ± 1% 0.60µs ± 7% -49.05% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_8-16 1.05µs ± 1% 0.61µs ± 9% -42.30% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_9-16 919ns ± 4% 643ns ±29% -30.02% (p=0.008 n=5+5) IntervalSklAdd/size_1-16 2.31µs ± 1% 1.77µs ± 2% -23.35% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_10-16 775ns ± 9% 603ns ± 7% -22.21% (p=0.008 n=5+5) IntervalSklAdd/size_10-16 3.96µs ± 2% 3.21µs ± 2% -18.80% (p=0.008 n=5+5) IntervalSklAdd/size_10000-16 4.31µs ± 1% 3.52µs ± 2% -18.43% (p=0.008 n=5+5) IntervalSklAdd/size_1000-16 3.96µs ± 2% 3.32µs ± 3% -16.21% (p=0.008 n=5+5) IntervalSklAdd/size_100-16 3.85µs ± 1% 3.25µs ± 4% -15.51% (p=0.008 n=5+5) IntervalSklAdd/size_1000000-16 8.02µs ± 3% 7.28µs ± 1% -9.27% (p=0.008 n=5+5) IntervalSklAdd/size_10000000-16 16.5µs ± 2% 15.8µs ± 2% -3.91% (p=0.008 n=5+5) IntervalSklAdd/size_100000-16 5.44µs ± 9% 4.82µs ±14% ~ (p=0.095 n=5+5) IntervalSklAdd/size_100000000-16 42.1µs ± 4% 41.4µs ± 2% ~ (p=0.421 n=5+5) name old alloc/op new alloc/op delta IntervalSklAddAndLookup/frac_0-16 789B ± 4% 0B -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_1-16 716B ± 3% 0B -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_2-16 632B ± 2% 0B -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_3-16 565B ± 5% 0B -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_4-16 473B ± 2% 0B -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_5-16 323B ±74% 0B -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_6-16 75.8B ± 1% 0.0B -100.00% (p=0.000 n=4+5) IntervalSklAddAndLookup/frac_7-16 65.0B ± 0% 0.0B -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_8-16 55.4B ± 1% 0.0B -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_9-16 45.6B ± 1% 0.0B -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_10-16 36.0B ± 0% 0.0B -100.00% (p=0.008 n=5+5) IntervalSklAdd/size_1-16 240B ± 2% 99B ± 0% -58.82% (p=0.008 n=5+5) IntervalSklAdd/size_10-16 373B ± 7% 202B ± 2% -45.73% (p=0.008 n=5+5) IntervalSklAdd/size_100-16 339B ± 4% 188B ± 3% -44.52% (p=0.008 n=5+5) IntervalSklAdd/size_10000-16 526B ± 8% 302B ±23% -42.63% (p=0.008 n=5+5) IntervalSklAdd/size_1000-16 365B ± 5% 215B ± 3% -41.02% (p=0.008 n=5+5) IntervalSklAdd/size_100000-16 919B ± 8% 763B ± 4% -16.99% (p=0.008 n=5+5) IntervalSklAdd/size_1000000-16 2.25kB ± 4% 1.92kB ± 6% -14.45% (p=0.008 n=5+5) IntervalSklAdd/size_10000000-16 6.11kB ± 6% 5.65kB ± 9% ~ (p=0.056 n=5+5) IntervalSklAdd/size_100000000-16 16.7kB ± 5% 17.0kB ±12% ~ (p=0.841 n=5+5) name old allocs/op new allocs/op delta IntervalSklAdd/size_1-16 6.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAdd/size_10-16 6.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAdd/size_100-16 6.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAdd/size_1000-16 6.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAdd/size_10000-16 6.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAdd/size_100000-16 6.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAdd/size_1000000-16 6.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAdd/size_10000000-16 6.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAdd/size_100000000-16 6.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_0-16 6.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_1-16 5.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_2-16 5.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_3-16 4.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_4-16 4.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_5-16 3.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_6-16 3.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_7-16 3.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_8-16 2.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_9-16 2.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) IntervalSklAddAndLookup/frac_10-16 2.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5) ```
This commit switches the byte encoding of a cacheValue from a custom encoding to an encoding that directly matches the in-memory representation of the struct. This allows for encoding and decoding to use an unsafe re-interpretation of the struct as a bytes slice, eliminating all bounds checks and byte assignments and replacing them with a single memcpy. The effect of this is dramatically faster encoding and decoding time in exchange for a slightly larger encoded size. Encoding time (run once when adding to the timestamp cache) drops from **9ns** to **.3ns**. Decoding time (run once per conflicting key when adding and reading to the timestamp cache) drops from **16ns** to **3ns**. In exchange, the actual encoded byte size increases from 29 bytes to 32 bytes. This isn't great as it means that skiplist pages will fill up **10%** faster, but that seems well worth the gains in encoding/decoding speed. ``` name old time/op new time/op delta IntervalSklEncodeValue/logical=false,synthetic=true,txnID=true-16 9.27ns ±47% 0.32ns ±37% -96.58% (p=0.008 n=5+5) IntervalSklEncodeValue/logical=true,synthetic=true,txnID=true-16 9.66ns ±54% 0.33ns ±37% -96.57% (p=0.008 n=5+5) IntervalSklEncodeValue/logical=true,synthetic=false,txnID=true-16 9.31ns ±40% 0.32ns ±42% -96.56% (p=0.008 n=5+5) IntervalSklEncodeValue/logical=true,synthetic=true,txnID=false-16 8.73ns ±31% 0.30ns ±39% -96.53% (p=0.008 n=5+5) IntervalSklEncodeValue/logical=true,synthetic=false,txnID=false-16 8.93ns ±34% 0.32ns ±28% -96.44% (p=0.008 n=5+5) IntervalSklEncodeValue/logical=false,synthetic=false,txnID=true-16 9.03ns ±33% 0.32ns ±50% -96.42% (p=0.008 n=5+5) IntervalSklEncodeValue/logical=false,synthetic=true,txnID=false-16 9.14ns ±32% 0.33ns ±35% -96.36% (p=0.008 n=5+5) IntervalSklEncodeValue/logical=false,synthetic=false,txnID=false-16 8.72ns ±27% 0.33ns ±43% -96.17% (p=0.008 n=5+5) IntervalSklDecodeValue/logical=true,synthetic=false,txnID=true-16 16.5ns ±30% 2.8ns ±28% -82.77% (p=0.008 n=5+5) IntervalSklDecodeValue/logical=false,synthetic=true,txnID=true-16 16.0ns ±28% 2.8ns ±25% -82.73% (p=0.008 n=5+5) IntervalSklDecodeValue/logical=true,synthetic=true,txnID=true-16 16.3ns ±36% 2.8ns ±36% -82.64% (p=0.008 n=5+5) IntervalSklDecodeValue/logical=false,synthetic=false,txnID=false-16 16.4ns ±25% 2.9ns ±31% -82.59% (p=0.008 n=5+5) IntervalSklDecodeValue/logical=false,synthetic=true,txnID=false-16 16.2ns ±33% 2.8ns ±29% -82.52% (p=0.008 n=5+5) IntervalSklDecodeValue/logical=false,synthetic=false,txnID=true-16 16.2ns ±31% 2.8ns ±29% -82.43% (p=0.008 n=5+5) IntervalSklDecodeValue/logical=true,synthetic=false,txnID=false-16 15.9ns ±25% 2.8ns ±28% -82.20% (p=0.008 n=5+5) IntervalSklDecodeValue/logical=true,synthetic=true,txnID=false-16 16.4ns ±35% 3.1ns ±60% -81.25% (p=0.008 n=5+5) name old alloc/op new alloc/op delta IntervalSklDecodeValue/logical=false,synthetic=false,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=false,synthetic=false,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=false,synthetic=true,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=false,synthetic=true,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=false,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=false,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=true,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=true,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=false,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=false,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=true,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=true,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=false,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=false,txnID=true-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=true,txnID=false-16 0.00B 0.00B ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=true,txnID=true-16 0.00B 0.00B ~ (all equal) name old allocs/op new allocs/op delta IntervalSklDecodeValue/logical=false,synthetic=false,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=false,synthetic=false,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=false,synthetic=true,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=false,synthetic=true,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=false,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=false,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=true,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklDecodeValue/logical=true,synthetic=true,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=false,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=false,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=true,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=false,synthetic=true,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=false,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=false,txnID=true-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=true,txnID=false-16 0.00 0.00 ~ (all equal) IntervalSklEncodeValue/logical=true,synthetic=true,txnID=true-16 0.00 0.00 ~ (all equal) ``` We see these effects in the rest of the timestamp cache benchmarks. In the benchmarks with the most overlaps (most calls to decodeValue per op) we see up to a **22%** speedup. Meanwhile, over the lifetime of those benchmarks, we see up to a **21%** increase in total allocated bytes (amortized across all ops). ``` name old time/op new time/op delta IntervalSklAdd/size_100000000-16 41.4µs ± 2% 32.3µs ± 7% -22.12% (p=0.000 n=10+9) IntervalSklAdd/size_10000000-16 16.2µs ± 1% 12.7µs ± 4% -21.57% (p=0.000 n=9+10) IntervalSklAdd/size_1000000-16 7.33µs ± 1% 6.18µs ± 4% -15.62% (p=0.000 n=9+10) TimestampCacheInsertion-16 1.96µs ± 5% 1.72µs ± 6% -12.06% (p=0.000 n=10+10) IntervalSklAdd/size_100000-16 4.63µs ± 2% 4.18µs ± 1% -9.92% (p=0.000 n=10+8) IntervalSklAdd/size_1000-16 3.32µs ± 3% 3.16µs ± 3% -4.97% (p=0.000 n=10+10) IntervalSklAdd/size_100-16 3.21µs ± 2% 3.06µs ± 4% -4.84% (p=0.000 n=10+10) IntervalSklAdd/size_10-16 3.19µs ± 3% 3.05µs ± 3% -4.25% (p=0.000 n=10+10) IntervalSklAdd/size_10000-16 3.52µs ± 3% 3.43µs ± 3% -2.47% (p=0.005 n=10+10) IntervalSklAdd/size_1-16 1.70µs ± 1% 1.67µs ± 3% -1.57% (p=0.016 n=9+10) IntervalSklAddAndLookup/frac_0-16 592ns ± 6% 509ns ±33% ~ (p=0.469 n=10+10) IntervalSklAddAndLookup/frac_1-16 592ns ± 8% 509ns ±32% ~ (p=0.424 n=10+10) IntervalSklAddAndLookup/frac_2-16 590ns ± 5% 510ns ±33% ~ (p=0.481 n=10+10) IntervalSklAddAndLookup/frac_3-16 585ns ± 5% 514ns ±32% ~ (p=0.484 n=9+10) IntervalSklAddAndLookup/frac_4-16 592ns ± 4% 511ns ±34% ~ (p=0.483 n=9+10) IntervalSklAddAndLookup/frac_5-16 591ns ± 8% 504ns ±35% ~ (p=0.165 n=10+10) IntervalSklAddAndLookup/frac_6-16 586ns ± 6% 508ns ±36% ~ (p=0.210 n=10+10) IntervalSklAddAndLookup/frac_7-16 588ns ± 6% 516ns ±46% ~ (p=0.344 n=9+10) IntervalSklAddAndLookup/frac_8-16 593ns ± 5% 521ns ±54% ~ (p=0.240 n=10+10) IntervalSklAddAndLookup/frac_9-16 594ns ± 5% 546ns ±54% ~ (p=0.739 n=10+10) IntervalSklAddAndLookup/frac_10-16 595ns ± 4% 525ns ±64% ~ (p=0.138 n=10+10) name old alloc/op new alloc/op delta IntervalSklAdd/size_1-16 99.0B ± 0% 99.0B ± 0% ~ (all equal) IntervalSklAdd/size_10-16 201B ± 5% 217B ±17% ~ (p=0.986 n=10+10) IntervalSklAdd/size_1000-16 243B ±26% 220B ±15% ~ (p=0.913 n=10+8) IntervalSklAdd/size_10000-16 325B ±19% 352B ±24% ~ (p=0.159 n=10+10) IntervalSklAdd/size_100000-16 715B ±26% 737B ±23% ~ (p=0.670 n=10+10) IntervalSklAdd/size_1000000-16 1.92kB ±12% 1.93kB ±20% ~ (p=0.781 n=10+10) IntervalSklAddAndLookup/frac_0-16 0.00B 0.00B ~ (all equal) IntervalSklAddAndLookup/frac_1-16 0.00B 0.00B ~ (all equal) IntervalSklAddAndLookup/frac_2-16 0.00B 0.00B ~ (all equal) IntervalSklAddAndLookup/frac_3-16 0.00B 0.00B ~ (all equal) IntervalSklAddAndLookup/frac_4-16 0.00B 0.00B ~ (all equal) IntervalSklAddAndLookup/frac_5-16 0.00B 0.00B ~ (all equal) IntervalSklAddAndLookup/frac_6-16 0.00B 0.00B ~ (all equal) IntervalSklAddAndLookup/frac_7-16 0.00B 0.00B ~ (all equal) IntervalSklAddAndLookup/frac_8-16 0.00B 0.00B ~ (all equal) IntervalSklAddAndLookup/frac_9-16 0.00B 0.00B ~ (all equal) IntervalSklAddAndLookup/frac_10-16 0.00B 0.00B ~ (all equal) IntervalSklAdd/size_100000000-16 16.3kB ± 4% 17.3kB ± 2% +5.85% (p=0.000 n=9+9) IntervalSklAdd/size_10000000-16 5.47kB ± 8% 5.83kB ±10% +6.53% (p=0.010 n=9+10) TimestampCacheInsertion-16 631B ± 2% 677B ± 3% +7.21% (p=0.000 n=10+10) IntervalSklAdd/size_100-16 194B ± 6% 236B ±28% +21.71% (p=0.012 n=10+10) name old allocs/op new allocs/op delta TimestampCacheInsertion-16 8.00 ± 0% 8.00 ± 0% ~ (all equal) IntervalSklAdd/size_1-16 0.00 0.00 ~ (all equal) IntervalSklAdd/size_10-16 0.00 0.00 ~ (all equal) IntervalSklAdd/size_100-16 0.00 0.00 ~ (all equal) IntervalSklAdd/size_1000-16 0.00 0.00 ~ (all equal) IntervalSklAdd/size_10000-16 0.00 0.00 ~ (all equal) IntervalSklAdd/size_100000-16 0.00 0.00 ~ (all equal) IntervalSklAdd/size_1000000-16 0.00 0.00 ~ (all equal) IntervalSklAdd/size_10000000-16 0.00 0.00 ~ (all equal) IntervalSklAdd/size_100000000-16 0.00 0.00 ~ (all equal) IntervalSklAddAndLookup/frac_0-16 0.00 0.00 ~ (all equal) IntervalSklAddAndLookup/frac_1-16 0.00 0.00 ~ (all equal) IntervalSklAddAndLookup/frac_2-16 0.00 0.00 ~ (all equal) IntervalSklAddAndLookup/frac_3-16 0.00 0.00 ~ (all equal) IntervalSklAddAndLookup/frac_4-16 0.00 0.00 ~ (all equal) IntervalSklAddAndLookup/frac_5-16 0.00 0.00 ~ (all equal) IntervalSklAddAndLookup/frac_6-16 0.00 0.00 ~ (all equal) IntervalSklAddAndLookup/frac_7-16 0.00 0.00 ~ (all equal) IntervalSklAddAndLookup/frac_8-16 0.00 0.00 ~ (all equal) IntervalSklAddAndLookup/frac_9-16 0.00 0.00 ~ (all equal) IntervalSklAddAndLookup/frac_10-16 0.00 0.00 ~ (all equal) ```
nvanbenschoten
force-pushed
the
nvanbenschoten/tscacheSyn
branch
from
c78b628
to
1db343c
Compare
nvanbenschoten
commented
Jan 5, 2021
There was a problem hiding this comment.
TFTR, and sorry for letting this hang! I addressed your comments here and in a separate PR. This should be good for a second look now.
Reviewable status:
pkg/kv/kvserver/tscache/cache.go, line 117 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
[nit, not this but] Name this return type.
Done.
pkg/kv/kvserver/tscache/cache.go, line 127 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
aside: seems worthwhile exporting a nicer looking symbol from the hlc package.
This aside inspired #58349. Should be better now.
pkg/kv/kvserver/tscache/interval_skl.go, line 806 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
[nit] Here and throughout, avoid "new" if we can.
Generally agree, though I want to keep this diff small so I'll hold off on this.
pkg/kv/kvserver/tscache/interval_skl.go, line 881 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Curious why this property is a useful one to have.
I added a bit more about this. The TL;DR of #58349 is that if either of two equal timestamps is non-synthetic, then the combination of them is not.
pkg/kv/kvserver/tscache/interval_skl.go, line 1231 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Mind making it clearer where this 12 is coming from? Ditto for decodeValue.
Done be ripping this up and replacing it with faster encoding/decoding routines in the 4th commit.
irfansharif
approved these changes
Jan 6, 2021
There was a problem hiding this comment.
Reviewed 1 of 3 files at r3, 2 of 3 files at r4, 2 of 2 files at r6.
Reviewable status:
pkg/kv/kvserver/tscache/cache.go, line 127 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
This aside inspired #58349. Should be better now.
I'll take full credit there.
|
TFTR! bors r+ |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #57683.
Before this change, a read with a synthetic timestamp would not pass the synthetic flag through the timestamp cache to conflicting writes. This could cause issues where a write that was bumped by one of these future timestamps would not notice that its write timestamp was now synthetic.
This change fixes this by propagating the synthetic flag through the timestamp cache. We have to play a few tricks to make this efficient around
sklPage.ratchetMaxTimestamp, which attempts to keeping a running maximum of the timestamps in a given skiplist using atomics. The added complexity here is encapsulated in a newratchetingTimetype.The testing around here is actually pretty solid.
TestIntervalSklConcurrencyandTestIntervalSklConcurrentVsSequentialboth perform long series of randomized operations and assert against ratchet inversions. This change adds some randomization around synthetic timestamps to the tests and introduces synthetic timestamps to the ratcheting policy (which wasn't otherwise necessary) to get solid randomized test coverage.This made encoding and decoding timestamp cache values slightly slower, but on the order of nanoseconds.