Skip to content

Commit

Permalink
Merge #66374
Browse files Browse the repository at this point in the history 
66374: kv: don't embed RangeDescriptor and Lease in EvictionToken, avoid routing allocs r=nvanbenschoten a=nvanbenschoten

This PR contains a series of changes to avoid heap allocations in the hot path of KV processing.

### kv: avoid heap allocation for single-range routing

For request's that only contain a single "part", we avoid the heap allocation in `DistSender.Send`.

### kv: don't embed RangeDescriptor and Lease in EvictionToken

This commit switches the handling of the RangeDescriptor and Lease fields in EvictionToken to be stored by reference, pointing into an immutable `CacheEntry` object instead of copying and storing these fields by value. This change is made because storing the fields by value was causing the object to escape to the heap all over the place. It was too easy to Call `Desc()` or `Leaseholder()` and then have this reference escape, which meant that we were allocating a single `EvictionToken` multiple times throughout the DistSender stack.

A secondary benefit of this change is that it removes memory copies and shrinks the size of the EvictionToken from 224 bytes to 40 bytes.

----

These changes combine to have the following impact:

```
name                            old time/op    new time/op    delta
KV/Scan/Native/rows=1-16          30.8µs ± 9%    29.0µs ± 3%  -5.98%  (p=0.000 n=10+9)
KV/Scan/Native/rows=100-16        53.2µs ± 2%    51.4µs ± 1%  -3.24%  (p=0.000 n=9+8)
KV/Scan/Native/rows=10-16         32.9µs ± 3%    32.0µs ± 4%  -2.84%  (p=0.004 n=9+10)
KV/Update/Native/rows=1-16         155µs ± 4%     152µs ± 3%  -1.80%  (p=0.024 n=9+9)
KV/Scan/Native/rows=1000-16        251µs ± 2%     247µs ± 2%  -1.54%  (p=0.046 n=8+9)
KV/Insert/Native/rows=1-16        97.7µs ± 4%    98.7µs ± 3%    ~     (p=0.133 n=9+10)
KV/Insert/Native/rows=10-16        140µs ± 2%     140µs ± 3%    ~     (p=0.661 n=10+9)
KV/Insert/Native/rows=100-16       503µs ± 4%     500µs ± 4%    ~     (p=0.739 n=10+10)
KV/Insert/Native/rows=1000-16     3.85ms ± 5%    3.94ms ± 7%    ~     (p=0.143 n=10+10)
KV/Insert/Native/rows=10000-16    42.8ms ± 9%    41.5ms ± 6%    ~     (p=0.280 n=10+10)
KV/Update/Native/rows=10-16        335µs ± 3%     333µs ± 2%    ~     (p=0.243 n=10+9)
KV/Update/Native/rows=100-16      1.87ms ± 9%    1.85ms ± 6%    ~     (p=0.842 n=10+9)
KV/Update/Native/rows=1000-16     15.9ms ± 5%    16.3ms ± 6%    ~     (p=0.105 n=10+10)
KV/Update/Native/rows=10000-16     130ms ± 2%     133ms ± 7%    ~     (p=0.122 n=8+10)
KV/Delete/Native/rows=1-16        98.4µs ± 3%    98.6µs ± 3%    ~     (p=0.971 n=10+10)
KV/Delete/Native/rows=10-16        154µs ± 2%     154µs ± 2%    ~     (p=0.645 n=8+8)
KV/Delete/Native/rows=100-16       601µs ± 3%     608µs ± 3%    ~     (p=0.222 n=9+9)
KV/Delete/Native/rows=1000-16     4.92ms ± 6%    5.00ms ±12%    ~     (p=0.796 n=10+10)
KV/Delete/Native/rows=10000-16    51.8ms ±10%    51.4ms ± 9%    ~     (p=0.796 n=10+10)
KV/Scan/Native/rows=10000-16      2.13ms ± 5%    2.14ms ± 5%    ~     (p=0.912 n=10+10)

name                            old alloc/op   new alloc/op   delta
KV/Scan/Native/rows=1-16          7.15kB ± 0%    6.46kB ± 0%  -9.68%  (p=0.000 n=10+10)
KV/Scan/Native/rows=10-16         8.56kB ± 0%    7.87kB ± 0%  -8.08%  (p=0.000 n=10+9)
KV/Update/Native/rows=1-16        21.7kB ± 1%    20.3kB ± 1%  -6.38%  (p=0.000 n=10+10)
KV/Insert/Native/rows=1-16        14.7kB ± 1%    14.0kB ± 0%  -5.09%  (p=0.000 n=10+9)
KV/Delete/Native/rows=1-16        14.2kB ± 1%    13.6kB ± 2%  -4.28%  (p=0.000 n=10+10)
KV/Scan/Native/rows=100-16        21.1kB ± 0%    20.4kB ± 0%  -3.25%  (p=0.000 n=10+10)
KV/Update/Native/rows=10-16       64.8kB ± 0%    63.2kB ± 0%  -2.34%  (p=0.000 n=9+9)
KV/Delete/Native/rows=10-16       35.1kB ± 1%    34.4kB ± 0%  -2.15%  (p=0.000 n=10+10)
KV/Insert/Native/rows=10-16       38.9kB ± 1%    38.2kB ± 1%  -1.80%  (p=0.000 n=10+10)
KV/Insert/Native/rows=100-16       277kB ± 1%     275kB ± 1%  -0.74%  (p=0.011 n=10+10)
KV/Update/Native/rows=100-16       485kB ± 0%     483kB ± 1%  -0.43%  (p=0.002 n=10+10)
KV/Scan/Native/rows=1000-16        172kB ± 0%     171kB ± 0%  -0.42%  (p=0.000 n=9+10)
KV/Scan/Native/rows=10000-16      1.51MB ± 0%    1.51MB ± 0%  -0.06%  (p=0.000 n=9+10)
KV/Insert/Native/rows=1000-16     2.54MB ± 1%    2.54MB ± 0%    ~     (p=0.684 n=10+10)
KV/Insert/Native/rows=10000-16    33.9MB ± 1%    34.1MB ± 2%    ~     (p=0.146 n=8+10)
KV/Update/Native/rows=1000-16     4.57MB ± 1%    4.56MB ± 1%    ~     (p=0.123 n=10+10)
KV/Update/Native/rows=10000-16    64.5MB ± 1%    64.7MB ± 2%    ~     (p=0.739 n=10+10)
KV/Delete/Native/rows=100-16       241kB ± 0%     240kB ± 1%    ~     (p=0.143 n=10+10)
KV/Delete/Native/rows=1000-16     2.22MB ± 1%    2.21MB ± 1%    ~     (p=0.113 n=9+10)
KV/Delete/Native/rows=10000-16    30.0MB ± 2%    30.1MB ± 1%    ~     (p=0.165 n=10+10)

name                            old allocs/op  new allocs/op  delta
KV/Scan/Native/rows=1-16            52.0 ± 0%      48.0 ± 0%  -7.69%  (p=0.000 n=10+10)
KV/Scan/Native/rows=10-16           56.0 ± 0%      52.0 ± 0%  -7.14%  (p=0.000 n=10+9)
KV/Scan/Native/rows=100-16          60.0 ± 0%      56.0 ± 0%  -6.67%  (p=0.000 n=10+10)
KV/Scan/Native/rows=1000-16         69.0 ± 0%      65.0 ± 0%  -5.80%  (p=0.000 n=9+9)
KV/Update/Native/rows=1-16           181 ± 0%       173 ± 0%  -4.42%  (p=0.000 n=10+10)
KV/Scan/Native/rows=10000-16        96.3 ± 1%      92.4 ± 4%  -4.00%  (p=0.000 n=8+10)
KV/Delete/Native/rows=1-16           116 ± 0%       112 ± 0%  -3.45%  (p=0.000 n=10+9)
KV/Insert/Native/rows=1-16           117 ± 0%       113 ± 0%  -3.42%  (p=0.001 n=8+9)
KV/Update/Native/rows=10-16          442 ± 0%       434 ± 0%  -1.76%  (p=0.000 n=8+9)
KV/Delete/Native/rows=10-16          235 ± 0%       231 ± 0%  -1.70%  (p=0.000 n=9+10)
KV/Insert/Native/rows=10-16          264 ± 0%       260 ± 0%  -1.52%  (p=0.000 n=10+9)
KV/Delete/Native/rows=100-16       1.26k ± 0%     1.25k ± 0%  -0.33%  (p=0.000 n=7+10)
KV/Update/Native/rows=100-16       2.65k ± 0%     2.64k ± 0%  -0.32%  (p=0.000 n=10+10)
KV/Insert/Native/rows=100-16       1.56k ± 0%     1.55k ± 0%  -0.28%  (p=0.000 n=10+6)
KV/Update/Native/rows=1000-16      25.9k ± 0%     25.9k ± 0%  -0.05%  (p=0.001 n=10+10)
KV/Delete/Native/rows=1000-16      11.3k ± 0%     11.3k ± 0%  -0.04%  (p=0.012 n=10+9)
KV/Insert/Native/rows=1000-16      14.3k ± 0%     14.3k ± 0%  -0.03%  (p=0.030 n=10+10)
KV/Insert/Native/rows=10000-16      141k ± 0%      141k ± 0%    ~     (p=0.839 n=10+10)
KV/Update/Native/rows=10000-16      263k ± 0%      262k ± 0%    ~     (p=0.101 n=10+10)
KV/Delete/Native/rows=10000-16      111k ± 0%      111k ± 0%    ~     (p=0.382 n=10+10)
```

Co-authored-by: Nathan VanBenschoten <[email protected]>
  • Loading branch information
@nvanbenschoten
craig[bot] and nvanbenschoten committed Jun 25, 2021
2 parents 6b9d2a1 + b1d702c commit 12aaefc
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 15 deletions.
3 changes: 2 additions & 1 deletion pkg/kv/kvclient/kvcoord/dist_sender.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,6 @@ func (ds *DistSender) Send(
ctx, sp := tracing.EnsureChildSpan(ctx, ds.AmbientContext.Tracer, "dist sender send")
defer sp.Finish()

var rplChunks []*roachpb.BatchResponse
splitET := false
var require1PC bool
lastReq := ba.Requests[len(ba.Requests)-1].GetInner()
Expand All @@ -724,6 +723,8 @@ func (ds *DistSender) Send(
log.Fatalf(ctx, "batch with MaxSpanRequestKeys=%d, TargetBytes=%d needs splitting",
log.Safe(ba.MaxSpanRequestKeys), log.Safe(ba.TargetBytes))
}
var singleRplChunk [1]*roachpb.BatchResponse
rplChunks := singleRplChunk[:0:1]

errIdxOffset := 0
for len(parts) > 0 {
Expand Down
49 changes: 35 additions & 14 deletions pkg/kv/kvclient/rangecache/range_cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,8 +211,8 @@ type EvictionToken struct {
// methods re-synchronize with the cache. However, it it changes, the
// descriptor only changes to other "compatible" descriptors (same range id
// and key bounds).
desc roachpb.RangeDescriptor
lease roachpb.Lease
desc *roachpb.RangeDescriptor
lease *roachpb.Lease
closedts roachpb.RangeClosedTimestampPolicy

// speculativeDesc, if not nil, is the descriptor that should replace desc if
Expand Down Expand Up @@ -250,8 +250,8 @@ func (rc *RangeCache) makeEvictionToken(
}
return EvictionToken{
rdc: rc,
desc: entry.desc,
lease: entry.lease,
desc: entry.Desc(),
lease: entry.leaseEvenIfSpeculative(),
closedts: entry.closedts,
speculativeDesc: speculativeDesc,
}
Expand Down Expand Up @@ -288,7 +288,7 @@ func (et EvictionToken) Desc() *roachpb.RangeDescriptor {
if !et.Valid() {
return nil
}
return &et.desc
return et.desc
}

// Leaseholder returns the cached leaseholder. If the cache didn't have any
Expand All @@ -297,7 +297,7 @@ func (et EvictionToken) Desc() *roachpb.RangeDescriptor {
// If a leaseholder is returned, it will correspond to one of the replicas in
// et.Desc().
func (et EvictionToken) Leaseholder() *roachpb.ReplicaDescriptor {
if et.lease.Empty() {
if !et.Valid() || et.lease == nil {
return nil
}
return &et.lease.Replica
Expand All @@ -309,6 +309,9 @@ func (et EvictionToken) LeaseSeq() roachpb.LeaseSequence {
if !et.Valid() {
panic("invalid LeaseSeq() call on empty EvictionToken")
}
if et.lease == nil {
return 0
}
return et.lease.Sequence
}

Expand All @@ -334,8 +337,8 @@ func (et *EvictionToken) syncRLocked(
et.clear()
return false, nil, nil
}
et.desc = cachedEntry.desc
et.lease = cachedEntry.lease
et.desc = cachedEntry.Desc()
et.lease = cachedEntry.leaseEvenIfSpeculative()
return true, cachedEntry, rawEntry
}

Expand Down Expand Up @@ -382,8 +385,8 @@ func (et *EvictionToken) UpdateLease(ctx context.Context, l *roachpb.Lease) bool
return false
}
if newEntry != nil {
et.desc = newEntry.desc
et.lease = newEntry.lease
et.desc = newEntry.Desc()
et.lease = newEntry.leaseEvenIfSpeculative()
} else {
// newEntry == nil means the lease is not compatible with the descriptor.
et.clear()
Expand Down Expand Up @@ -424,7 +427,7 @@ func (et *EvictionToken) EvictLease(ctx context.Context) {
et.rdc.rangeCache.Lock()
defer et.rdc.rangeCache.Unlock()

if et.lease.Empty() {
if et.lease == nil {
log.Fatalf(ctx, "attempting to clear lease from cache entry without lease")
}

Expand All @@ -437,8 +440,8 @@ func (et *EvictionToken) EvictLease(ctx context.Context) {
if !ok {
return
}
et.desc = newEntry.desc
et.lease = newEntry.lease
et.desc = newEntry.Desc()
et.lease = newEntry.leaseEvenIfSpeculative()
et.rdc.swapEntryLocked(ctx, rawEntry, newEntry)
}

Expand Down Expand Up @@ -524,7 +527,15 @@ func (rc *RangeCache) Lookup(ctx context.Context, key roachpb.RKey) (CacheEntry,
if err != nil {
return CacheEntry{}, err
}
return CacheEntry{tok.desc, tok.lease, tok.closedts}, nil
var e CacheEntry
if tok.desc != nil {
e.desc = *tok.desc
}
if tok.lease != nil {
e.lease = *tok.lease
}
e.closedts = tok.closedts
return e, nil
}

// GetCachedOverlapping returns all the cached entries which overlap a given
Expand Down Expand Up @@ -1118,6 +1129,16 @@ func (e *CacheEntry) Lease() *roachpb.Lease {
return &e.lease
}

// leaseEvenIfSpeculative is like Lease, except it returns a Lease object even
// if that lease is speculative. Returns nil if no speculative or non-speculative
// lease is known.
func (e *CacheEntry) leaseEvenIfSpeculative() *roachpb.Lease {
if e.lease.Empty() {
return nil
}
return &e.lease
}

// ClosedTimestampPolicy returns the cached understanding of the range's closed
// timestamp policy. If no policy is known, LAG_BY_CLUSTER_SETTING is returned.
func (e *CacheEntry) ClosedTimestampPolicy() roachpb.RangeClosedTimestampPolicy {
Expand Down

0 comments on commit 12aaefc

Please sign in to comment.