kv: perform local calls in a goroutine

[petermattis]

Perform local calls in a goroutine so that they can be cancelled and so
that they don't block the caller indefinitely.

See #10427

---

This change is 

[petermattis]

I'm going to try this PR on delta sans #10931.

[tamird]

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

pkg/kv/transport.go, line 184 at r1 (raw file):

		}

		go func() {

it would be mildly cleaner to share this with the below go routine

batchFn := client.client.Batch
if localServer := gt.rpcContext.GetLocalInternalServerForAddr(addr); enableLocalCalls && localServer != nil {
		batchFn = localServer.Batch
		// Clone the request. At the time of writing, Replica may mutate it
		// during command execution which can lead to data races.
		//
		// TODO(tamird): we should clone all of client.args.Header, but the
		// assertions in protoutil.Clone fire and there seems to be no
		// reasonable workaround.
		origTxn := client.args.Txn
		if origTxn != nil {
			clonedTxn := origTxn.Clone()
			client.args.Txn = &clonedTxn
		}
}
go func() {
  reply, err := batchFn(..)
  ...
}()

---

Comments from Reviewable

[petermattis]

Review status: 1 of 3 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/storage/replica.go, line 1876 at r2 (raw file):

				// Synchronously process any intents that need resolving here in order
				// to apply back pressure on the client which generated them.
				r.store.intentResolver.processIntents(r, propResult.Intents)

This is actually semi-synchronous as it uses RunLimitedAsyncTask internally.

---

Comments from Reviewable

[petermattis]

I'm going to push a build with this second commit to delta.

---

Review status: 1 of 3 files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[tamird]

second commit seems good, but hard to predict what effect it will have!

---

Reviewed 2 of 2 files at r3, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.

---

pkg/storage/replica.go, line 1669 at r4 (raw file):

	}
	if result.Local.intents != nil && len(*result.Local.intents) > 0 {
		log.Eventf(ctx, "submitting %d intents to asynchronous processing",

this has rotted

---

pkg/storage/replica.go, line 1671 at r4 (raw file):

		log.Eventf(ctx, "submitting %d intents to asynchronous processing",
			len(*result.Local.intents))
		r.store.intentResolver.processIntents(r, *result.Local.intents)

should this be result.Local.detachIntents()? if so the check above should probably be simplified: if intents := result.Local.detachIntents(); len(intents) > 0 { ...

---

pkg/storage/replica.go, line 1873 at r4 (raw file):

			// in processRaftCommand.
			endCmds = nil
			if propResult.Intents != nil {

any reason this doesn't check length?

---

pkg/storage/replica.go, line 4239 at r4 (raw file):

		// to nudge the intents in case they're expired; next time around we'll
		// hopefully have more luck.
		r.store.intentResolver.processIntents(r, *result.Local.intents)

should this be result.Local.detachIntents()? if so the check above should probably be simplified: if intents := result.Local.detachIntents(); len(intents) > 0 { ...

---

Comments from Reviewable

[petermattis]

Review status: 2 of 5 files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.

---

pkg/kv/transport.go, line 184 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

> it would be mildly cleaner to share this with the below go routine > ``` > batchFn := client.client.Batch > if localServer := gt.rpcContext.GetLocalInternalServerForAddr(addr); enableLocalCalls && localServer != nil { > batchFn = localServer.Batch > // Clone the request. At the time of writing, Replica may mutate it > // during command execution which can lead to data races. > // > // TODO(tamird): we should clone all of client.args.Header, but the > // assertions in protoutil.Clone fire and there seems to be no > // reasonable workaround. > origTxn := client.args.Txn > if origTxn != nil { > clonedTxn := origTxn.Clone() > client.args.Txn = &clonedTxn > } > } > go func() { > reply, err := batchFn(..) > ... > }() > ```

That

---

pkg/storage/replica.go, line 1669 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

> this has rotted

Its only sort of inaccurate. `processIntents` is semi-synchronous (or is that semi-asynchronous). I've renamed that method back to `processIntentsAsync` now that `processIntentsAsync` was no longer being used.

---

pkg/storage/replica.go, line 1671 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

> should this be `result.Local.detachIntents()`? if so the check above should probably be simplified: `if intents := result.Local.detachIntents(); len(intents) > 0 { ...`

Done.

---

pkg/storage/replica.go, line 1873 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

> any reason this doesn't check length?

Done.

---

pkg/storage/replica.go, line 4239 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

> should this be `result.Local.detachIntents()`? if so the check above should probably be simplified: `if intents := result.Local.detachIntents(); len(intents) > 0 { ...`

Done.

---

Comments from Reviewable

[petermattis]

Review status: 2 of 5 files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.

---

pkg/kv/transport.go, line 184 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

> That

`client.client.Batch` and `localServer.Batch` have different signatures.

---

Comments from Reviewable

[tamird]

---

Reviewed 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/kv/transport.go, line 184 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

> `client.client.Batch` and `localServer.Batch` have different signatures.

True, it would have to begin with ``` batchFn := func(ctx context.Context, args *roachpb.BatchRequest) error { return client.client.Batch(ctx, args) } ```

---

Comments from Reviewable

[spencerkimball]

I think maybe a more direct assertion here makes sense. Otherwise we're just going to assert the lResult is empty, and then only if shouldAssert is true. Might be better to call it out directly and add a comment.

[petermattis]

Call what out directly? That lResult.intents should be nil?

[spencerkimball]

Yes, suggestion was to make sure it's detached here with an assertion and a comment.

[petermattis]

Done.

[spencerkimball]

A problem is that this isn't always the path that a finished request follows. Original proposals which succeed but are re-proposed and cancelled commands will both leave intents to the GC. Maybe that's OK, although knowing our luck, there'll be some degenerate use case that creates massive amounts of intent garbage. After some thought I'm not coming up with any good solutions. However, let's add a comment to the effect that these kinds of requests will generate GC-ready intents.

[petermattis]

Comment added.

[spencerkimball]

Does this have any impact on the single-node benchmarks? Just curious; I think this is the right thing to do.

[petermattis]

Let me check.

[spencerkimball]

By the way, we're passing wait=true here to RunLimitedAsyncTask means that something which horks the intent queue will starve other work. I think you might consider what's being done DistSender.sendPartialBatchAsync. There, we pass wait=false to Stopper.RunLimitedAsyncTask, and on stop.ErrThrottled, we synchronously send the partial batch.

[petermattis]

Yeah, there isn't much point in not using the blocked goroutine. I'd like to do this in a follow-up PR, though. I'll file an issue.

[petermattis]

The change to local call handling is across the board negative for the single-node benchmarks:

name               old time/op  new time/op  delta
KVInsert1_SQL-8     341µs ± 3%   377µs ± 5%  +10.57%   (p=0.000 n=9+10)
KVInsert10_SQL-8    497µs ± 1%   521µs ± 2%   +4.74%    (p=0.000 n=8+9)
KVInsert100_SQL-8  1.49ms ± 1%  1.54ms ± 1%   +2.88%  (p=0.000 n=10+10)
KVUpdate1_SQL-8     510µs ± 2%   579µs ± 1%  +13.49%   (p=0.000 n=10+8)
KVUpdate10_SQL-8    758µs ± 1%   836µs ± 1%  +10.22%   (p=0.000 n=10+8)
KVUpdate100_SQL-8  2.96ms ± 3%  3.02ms ± 2%   +1.92%  (p=0.007 n=10+10)
KVDelete1_SQL-8     459µs ± 4%   513µs ± 4%  +11.74%  (p=0.000 n=10+10)
KVDelete10_SQL-8    664µs ± 1%   736µs ± 1%  +10.77%   (p=0.000 n=10+9)
KVDelete100_SQL-8  2.40ms ± 3%  2.50ms ± 1%   +3.87%   (p=0.000 n=10+8)
KVScan1_SQL-8       215µs ± 3%   227µs ± 1%   +5.62%   (p=0.000 n=10+9)
KVScan10_SQL-8      246µs ± 2%   252µs ± 1%   +2.37%   (p=0.000 n=10+9)
KVScan100_SQL-8     476µs ± 2%   518µs ± 1%   +8.80%  (p=0.000 n=10+10)

I'm actually somewhat surprised the effect is this large. And I expected the effect to diminish with larger batch sizes such as the 10 and 100 benchmarks. I also looked at disabling the local call optimization completely, but that is a 30% perf hit.

[spencerkimball]

Wow, that's really a nasty performance hit. On the other hand, optimizing this seems like a low priority.

[petermattis]

I'll file an issue about revisiting the performance hit. It is somewhat higher than I expected. And more variable than I expected. I would have expected an across-the-board impact on these benchmarks which are almost always performing either 1PC transactions or read-only operations. Why does KVScan1 only slow down by 12µs while KVDelete1 slows down by 54µs?

[bdarnell]

---

Reviewed 1 of 1 files at r1, 2 of 3 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

---

Comments from Reviewable