Skip to content

Commit e822649

Browse files
nvbnvb
committed
kv: pass BatchRequest by reference, not value
This commit switches the sender interface from: ```go type Sender interface { Send(context.Context, roachpb.BatchRequest) (*roachpb.BatchResponse, *roachpb.Error) } ``` to ```go type Sender interface { Send(context.Context, *roachpb.BatchRequest) (*roachpb.BatchResponse, *roachpb.Error) } ``` In doing so, the entire KV layer (client-side and server-side) switches from passing `BatchRequest` objects by pointer instead of by value. These objects are over 200 bytes in size and are passed to hundreds of functions throughout the KV layer in service of a single request, so this is a dramatic reduction in memory copies. Furthermore, these objects were often already escaping to the heap, so this actually reduces heap allocations by being more deliberate about when we heap allocate and when we perform a shallow clone. \### Macro benchmarks ``` name old ops/s new ops/s delta kv95/enc=false/nodes=3/cpu=32 120k ± 4% 122k ± 3% +2.28% (p=0.052 n=10+10) name old avg(ms) new avg(ms) delta kv95/enc=false/nodes=3/cpu=32 1.60 ± 0% 1.60 ± 0% ~ (all equal) name old p99(ms) new p99(ms) delta kv95/enc=false/nodes=3/cpu=32 7.79 ± 4% 7.60 ± 0% -2.44% (p=0.013 n=10+8) ``` This commit is based on #86957. Together, they have the following impact on macro benchmark performance: ``` name old ops/s new ops/s delta kv95/enc=false/nodes=3/cpu=32 109k ±10% 122k ± 3% +12.35% (p=0.000 n=10+10) name old avg(ms) new avg(ms) delta kv95/enc=false/nodes=3/cpu=32 1.79 ±12% 1.60 ± 0% -10.61% (p=0.000 n=10+8) name old p99(ms) new p99(ms) delta kv95/enc=false/nodes=3/cpu=32 8.53 ±17% 7.60 ± 0% -10.90% (p=0.000 n=10+8) ``` \### Micro benchmarks ``` name old time/op new time/op delta KV/Scan/Native/rows=1-10 17.1µs ± 1% 16.5µs ± 2% -3.36% (p=0.000 n=8+10) KV/Update/Native/rows=1-10 66.9µs ± 2% 65.2µs ± 2% -2.52% (p=0.000 n=9+10) KV/Insert/Native/rows=1-10 42.1µs ± 3% 41.3µs ± 3% -1.95% (p=0.015 n=10+10) KV/Delete/Native/rows=1-10 41.9µs ± 3% 41.2µs ± 2% -1.69% (p=0.035 n=10+10) KV/Insert/SQL/rows=1-10 125µs ± 2% 126µs ± 6% ~ (p=0.631 n=10+10) KV/Update/SQL/rows=1-10 171µs ± 4% 170µs ± 3% ~ (p=0.579 n=10+10) KV/Delete/SQL/rows=1-10 138µs ± 4% 138µs ± 4% ~ (p=0.393 n=10+10) KV/Scan/SQL/rows=1-10 94.4µs ± 4% 93.6µs ± 3% ~ (p=0.579 n=10+10) name old alloc/op new alloc/op delta KV/Update/Native/rows=1-10 22.3kB ± 0% 21.6kB ± 0% -2.83% (p=0.000 n=9+10) KV/Insert/Native/rows=1-10 15.8kB ± 0% 15.5kB ± 0% -1.86% (p=0.000 n=9+10) KV/Delete/Native/rows=1-10 15.5kB ± 1% 15.2kB ± 0% -1.83% (p=0.000 n=10+10) KV/Update/SQL/rows=1-10 51.6kB ± 0% 51.0kB ± 0% -1.11% (p=0.000 n=10+8) KV/Insert/SQL/rows=1-10 44.8kB ± 0% 44.4kB ± 0% -0.80% (p=0.000 n=10+9) KV/Scan/Native/rows=1-10 7.57kB ± 0% 7.52kB ± 0% -0.70% (p=0.000 n=9+10) KV/Delete/SQL/rows=1-10 51.6kB ± 0% 51.3kB ± 0% -0.54% (p=0.000 n=10+10) KV/Scan/SQL/rows=1-10 24.3kB ± 0% 24.3kB ± 0% -0.14% (p=0.009 n=8+10) name old allocs/op new allocs/op delta KV/Update/Native/rows=1-10 182 ± 0% 181 ± 0% -0.89% (p=0.000 n=9+10) KV/Delete/Native/rows=1-10 127 ± 0% 126 ± 0% -0.79% (p=0.000 n=10+10) KV/Insert/Native/rows=1-10 128 ± 0% 127 ± 0% -0.78% (p=0.000 n=9+10) KV/Update/SQL/rows=1-10 521 ± 0% 519 ± 0% -0.39% (p=0.000 n=10+8) KV/Delete/SQL/rows=1-10 386 ± 0% 385 ± 0% -0.26% (p=0.000 n=8+8) KV/Insert/SQL/rows=1-10 359 ± 0% 359 ± 0% -0.22% (p=0.009 n=10+10) KV/Scan/Native/rows=1-10 55.0 ± 0% 55.0 ± 0% ~ (all equal) KV/Scan/SQL/rows=1-10 281 ± 0% 281 ± 0% ~ (all equal) ``` Release justification: None. Don't merge until after the branch cut.
1 parent 3dd5e68 commit e822649

127 files changed

Lines changed: 870 additions & 837 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎pkg/ccl/backupccl/backup_intents_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ func TestCleanupIntentsDuringBackupPerformanceRegression(t *testing.T) {
4343

4444
// Interceptor catches requests that cleanup transactions of size 1000 which are
4545
// test data transactions. All other transaction commits pass though.
46-
interceptor := func(ctx context.Context, req roachpb.BatchRequest) *roachpb.Error {
46+
interceptor := func(ctx context.Context, req *roachpb.BatchRequest) *roachpb.Error {
4747
endTxn := req.Requests[0].GetEndTxn()
4848
if endTxn != nil && !endTxn.Commit && len(endTxn.LockSpans) == perTransactionRowCount {
4949
// If this is a rollback of one the test's SQL transactions, allow the

‎pkg/ccl/backupccl/backup_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6170,7 +6170,7 @@ func TestRestoreErrorPropagates(t *testing.T) {
61706170
jobsTableKey := keys.SystemSQLCodec.TablePrefix(uint32(systemschema.JobsTable.GetID()))
61716171
var shouldFail, failures int64
61726172
params.ServerArgs.Knobs.Store = &kvserver.StoreTestingKnobs{
6173-
TestingRequestFilter: func(ctx context.Context, ba roachpb.BatchRequest) *roachpb.Error {
6173+
TestingRequestFilter: func(ctx context.Context, ba *roachpb.BatchRequest) *roachpb.Error {
61746174
// Intercept Put and ConditionalPut requests to the jobs table
61756175
// and, if shouldFail is positive, increment failures and return an
61766176
// injected error.
@@ -6298,7 +6298,7 @@ func TestPaginatedBackupTenant(t *testing.T) {
62986298
r.EndKey.Equal(r.Key.PrefixEnd())
62996299
}
63006300
params.ServerArgs.Knobs.Store = &kvserver.StoreTestingKnobs{
6301-
TestingRequestFilter: func(ctx context.Context, request roachpb.BatchRequest) *roachpb.Error {
6301+
TestingRequestFilter: func(ctx context.Context, request *roachpb.BatchRequest) *roachpb.Error {
63026302
for _, ru := range request.Requests {
63036303
if exportRequest, ok := ru.GetInner().(*roachpb.ExportRequest); ok &&
63046304
!isLeasingExportRequest(exportRequest) {
@@ -6316,7 +6316,7 @@ func TestPaginatedBackupTenant(t *testing.T) {
63166316
}
63176317
return nil
63186318
},
6319-
TestingResponseFilter: func(ctx context.Context, ba roachpb.BatchRequest, br *roachpb.BatchResponse) *roachpb.Error {
6319+
TestingResponseFilter: func(ctx context.Context, ba *roachpb.BatchRequest, br *roachpb.BatchResponse) *roachpb.Error {
63206320
for i, ru := range br.Responses {
63216321
if exportRequest, ok := ba.Requests[i].GetInner().(*roachpb.ExportRequest); ok &&
63226322
!isLeasingExportRequest(exportRequest) {
@@ -7156,7 +7156,7 @@ func TestClientDisconnect(t *testing.T) {
71567156
blockBackupOrRestore(ctx)
71577157
}}},
71587158
Store: &kvserver.StoreTestingKnobs{
7159-
TestingResponseFilter: func(ctx context.Context, ba roachpb.BatchRequest, br *roachpb.BatchResponse) *roachpb.Error {
7159+
TestingResponseFilter: func(ctx context.Context, ba *roachpb.BatchRequest, br *roachpb.BatchResponse) *roachpb.Error {
71607160
for _, ru := range br.Responses {
71617161
switch ru.GetInner().(type) {
71627162
case *roachpb.ExportResponse:

‎pkg/ccl/changefeedccl/changefeed_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4830,7 +4830,7 @@ func TestChangefeedProtectedTimestamps(t *testing.T) {
48304830
}
48314831
}
48324832
requestFilter = kvserverbase.ReplicaRequestFilter(func(
4833-
ctx context.Context, ba roachpb.BatchRequest,
4833+
ctx context.Context, ba *roachpb.BatchRequest,
48344834
) *roachpb.Error {
48354835
if ba.Txn == nil || ba.Txn.Name != "changefeed backfill" {
48364836
return nil

‎pkg/ccl/kvccl/kvfollowerreadsccl/followerreads.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ func canSendToFollower(
134134
st *cluster.Settings,
135135
clock *hlc.Clock,
136136
ctPolicy roachpb.RangeClosedTimestampPolicy,
137-
ba roachpb.BatchRequest,
137+
ba *roachpb.BatchRequest,
138138
) bool {
139139
return kvserver.BatchCanBeEvaluatedOnFollower(ba) &&
140140
closedTimestampLikelySufficient(st, clock, ctPolicy, ba.RequiredFrontier()) &&

‎pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -108,25 +108,25 @@ func TestCanSendToFollower(t *testing.T) {
108108
txn.GlobalUncertaintyLimit = ts
109109
return txn
110110
}
111-
batch := func(txn *roachpb.Transaction, req roachpb.Request) roachpb.BatchRequest {
112-
var ba roachpb.BatchRequest
111+
batch := func(txn *roachpb.Transaction, req roachpb.Request) *roachpb.BatchRequest {
112+
ba := &roachpb.BatchRequest{}
113113
ba.Txn = txn
114114
ba.Add(req)
115115
return ba
116116
}
117-
withBatchTimestamp := func(ba roachpb.BatchRequest, ts hlc.Timestamp) roachpb.BatchRequest {
117+
withBatchTimestamp := func(ba *roachpb.BatchRequest, ts hlc.Timestamp) *roachpb.BatchRequest {
118118
ba.Timestamp = ts
119119
return ba
120120
}
121-
withServerSideBatchTimestamp := func(ba roachpb.BatchRequest, ts hlc.Timestamp) roachpb.BatchRequest {
121+
withServerSideBatchTimestamp := func(ba *roachpb.BatchRequest, ts hlc.Timestamp) *roachpb.BatchRequest {
122122
ba = withBatchTimestamp(ba, ts)
123123
ba.TimestampFromServerClock = (*hlc.ClockTimestamp)(&ts)
124124
return ba
125125
}
126126

127127
testCases := []struct {
128128
name string
129-
ba roachpb.BatchRequest
129+
ba *roachpb.BatchRequest
130130
ctPolicy roachpb.RangeClosedTimestampPolicy
131131
disabledEnterprise bool
132132
disabledFollowerReads bool
@@ -441,11 +441,13 @@ func TestCanSendToFollower(t *testing.T) {
441441
},
442442
{
443443
name: "non-enterprise",
444+
ba: withBatchTimestamp(batch(nil, &roachpb.GetRequest{}), stale),
444445
disabledEnterprise: true,
445446
exp: false,
446447
},
447448
{
448449
name: "follower reads disabled",
450+
ba: withBatchTimestamp(batch(nil, &roachpb.GetRequest{}), stale),
449451
disabledFollowerReads: true,
450452
exp: false,
451453
},

‎pkg/ccl/streamingccl/streamingest/stream_ingestion_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ func TestStreamIngestionJobWithRandomClient(t *testing.T) {
151151
},
152152
}
153153
params.ServerArgs.Knobs.Store = &kvserver.StoreTestingKnobs{
154-
TestingRequestFilter: func(_ context.Context, ba roachpb.BatchRequest) *roachpb.Error {
154+
TestingRequestFilter: func(_ context.Context, ba *roachpb.BatchRequest) *roachpb.Error {
155155
for _, req := range ba.Requests {
156156
switch r := req.GetInner().(type) {
157157
case *roachpb.RevertRangeRequest:

‎pkg/cli/zip_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ func TestUnavailableZip(t *testing.T) {
264264
close(closedCh)
265265
unavailableCh.Store(closedCh)
266266
knobs := &kvserver.StoreTestingKnobs{
267-
TestingRequestFilter: func(ctx context.Context, _ roachpb.BatchRequest) *roachpb.Error {
267+
TestingRequestFilter: func(ctx context.Context, _ *roachpb.BatchRequest) *roachpb.Error {
268268
select {
269269
case <-unavailableCh.Load().(chan struct{}):
270270
case <-ctx.Done():

‎pkg/internal/client/requestbatcher/batcher.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -526,8 +526,8 @@ func (b *batch) rangeID() roachpb.RangeID {
526526
return b.reqs[0].rangeID
527527
}
528528

529-
func (b *batch) batchRequest(cfg *Config) roachpb.BatchRequest {
530-
req := roachpb.BatchRequest{
529+
func (b *batch) batchRequest(cfg *Config) *roachpb.BatchRequest {
530+
req := &roachpb.BatchRequest{
531531
// Preallocate the Requests slice.
532532
Requests: make([]roachpb.RequestUnion, 0, len(b.reqs)),
533533
}

‎pkg/internal/client/requestbatcher/batcher_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,14 @@ type batchResp struct {
3737

3838
type batchSend struct {
3939
ctx context.Context
40-
ba roachpb.BatchRequest
40+
ba *roachpb.BatchRequest
4141
respChan chan<- batchResp
4242
}
4343

4444
type chanSender chan batchSend
4545

4646
func (c chanSender) Send(
47-
ctx context.Context, ba roachpb.BatchRequest,
47+
ctx context.Context, ba *roachpb.BatchRequest,
4848
) (*roachpb.BatchResponse, *roachpb.Error) {
4949
respChan := make(chan batchResp, 1)
5050
select {
@@ -501,7 +501,7 @@ func TestMaxKeysPerBatchReq(t *testing.T) {
501501
s := <-sc
502502
assert.Equal(t, int64(5), s.ba.MaxSpanRequestKeys)
503503
assert.Len(t, s.ba.Requests, 3)
504-
br := makeResp(&s.ba, spanMap{
504+
br := makeResp(s.ba, spanMap{
505505
{"d", "g"}: {"d", "g"},
506506
{"a", "d"}: {"c", "d"},
507507
{"b", "m"}: {"c", "m"},
@@ -513,7 +513,7 @@ func TestMaxKeysPerBatchReq(t *testing.T) {
513513
s = <-sc
514514
assert.Equal(t, int64(5), s.ba.MaxSpanRequestKeys)
515515
assert.Len(t, s.ba.Requests, 3)
516-
br = makeResp(&s.ba, spanMap{
516+
br = makeResp(s.ba, spanMap{
517517
{"d", "g"}: {"e", "g"},
518518
{"c", "d"}: nilResumeSpan,
519519
{"c", "m"}: {"e", "m"},
@@ -525,7 +525,7 @@ func TestMaxKeysPerBatchReq(t *testing.T) {
525525
s = <-sc
526526
assert.Equal(t, int64(5), s.ba.MaxSpanRequestKeys)
527527
assert.Len(t, s.ba.Requests, 2)
528-
br = makeResp(&s.ba, spanMap{
528+
br = makeResp(s.ba, spanMap{
529529
{"e", "g"}: nilResumeSpan,
530530
{"e", "m"}: {"h", "m"},
531531
})
@@ -536,7 +536,7 @@ func TestMaxKeysPerBatchReq(t *testing.T) {
536536
s = <-sc
537537
assert.Equal(t, int64(5), s.ba.MaxSpanRequestKeys)
538538
assert.Len(t, s.ba.Requests, 1)
539-
br = makeResp(&s.ba, spanMap{
539+
br = makeResp(s.ba, spanMap{
540540
{"h", "m"}: nilResumeSpan,
541541
})
542542
s.respChan <- batchResp{br: br}

‎pkg/jobs/jobs_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2728,7 +2728,7 @@ func TestStartableJobTxnRetry(t *testing.T) {
27282728
haveInjectedRetry := false
27292729
params := base.TestServerArgs{}
27302730
params.Knobs.Store = &kvserver.StoreTestingKnobs{
2731-
TestingRequestFilter: func(ctx context.Context, r roachpb.BatchRequest) *roachpb.Error {
2731+
TestingRequestFilter: func(ctx context.Context, r *roachpb.BatchRequest) *roachpb.Error {
27322732
if r.Txn == nil || r.Txn.Name != txnName {
27332733
return nil
27342734
}

0 commit comments

Comments
 (0)