client: Make tsoStream receives asynchronously #8483

MyonKeminta · 2024-08-02T08:01:39Z

What problem does this PR solve?

Issue Number: Ref #8432

What is changed and how does it work?

client: Make tsoStream receives asynchronously. This makes it possible to allow the tsoDispatcher send multiple requests and wait for their responses concurrently.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

ti-chi-bot · 2024-08-02T08:01:42Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: MyonKeminta <[email protected]>

codecov · 2024-09-04T09:31:49Z

Codecov Report

Attention: Patch coverage is 81.14754% with 46 lines in your changes missing coverage. Please review.

Project coverage is 77.61%. Comparing base (098b802) to head (5314485).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8483      +/-   ##
==========================================
- Coverage   77.66%   77.61%   -0.05%     
==========================================
  Files         474      474              
  Lines       61898    62033     +135     
==========================================
+ Hits        48072    48148      +76     
- Misses      10283    10340      +57     
- Partials     3543     3545       +2

Flag	Coverage Δ
unittests	`77.61% <81.14%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

client/tso_batch_controller.go

client/tso_dispatcher.go

client/tso_stream.go

Signed-off-by: MyonKeminta <[email protected]>

cfzjywxk · 2024-09-06T11:39:24Z

@JmPotato @lhy1024 PTAL, thx

lhy1024 · 2024-09-09T05:46:33Z

client/tso_stream_test.go

@@ -0,0 +1,333 @@
+// Copyright 2024 TiKV Project Authors.


Do we need to add bench for it?

Do you mean mocking the internal implementation of the stream and benching the overhead of the tsoStream layer?

I'd like to add a bench to compare the performance change before and after introducing this pr.

I'll have a try on this

lhy1024 · 2024-09-09T06:35:49Z

client/tso_stream.go

+
+const invalidStreamID = "<invalid>"
+
+// TODO: Pass a context?


Will we pass it?

I forgot this. I'll have a try.

lhy1024 · 2024-09-09T07:12:43Z

client/tso_dispatcher.go

 	})
+
+	// A large-enough capacity to hold maximum concurrent RPC requests. In our design, the concurrency is at most 16.


Will it be changed in the future?

I don't have such plan. As the more concurrent RPC there is, the less we can benefit from each additional concurrent RPC, it's not a good idea to allow too many concurrent RPCs for each stream. In our current plan, 16 is the maximum.

…make-tso-stream-async

Signed-off-by: MyonKeminta <[email protected]>

…make-tso-stream-async

Signed-off-by: MyonKeminta <[email protected]>

…make-tso-stream-async

Signed-off-by: MyonKeminta <[email protected]>

MyonKeminta · 2024-09-12T07:40:08Z

@lhy1024 I've done some benchmarks with the stream implemented by channels internally to show the overhead of tsoStream and tsoDispatcher. Please see if it's acceptable.

Before this PR: (code in #8618 , which is branched from the merge-base of this PR and master)

goos: linux
goarch: amd64
pkg: github.com/tikv/pd/client
cpu: Intel(R) Xeon(R) Silver 4214R CPU @ 2.40GHz
BenchmarkTSODispatcherHandleRequests-28           140149              7508 ns/op
BenchmarkTSOStreamSendRecv-28                    2788069               425.8 ns/op
PASS
ok      github.com/tikv/pd/client       2.796s

This PR (bff9eae):

goos: linux
goarch: amd64
pkg: github.com/tikv/pd/client
cpu: Intel(R) Xeon(R) Silver 4214R CPU @ 2.40GHz
BenchmarkTSODispatcherHandleRequests-28           119170              9254 ns/op
BenchmarkTSOStreamSendRecv-28                     676719              1647 ns/op
PASS
ok      github.com/tikv/pd/client       3.276s

There are several microseconds of additional overhead in this PR.

Please see if this is acceptable.

If you feel good with it, let's consider merging #8618 before this PR.

Signed-off-by: MyonKeminta <[email protected]>

…make-tso-stream-async

…Request from the pool Signed-off-by: MyonKeminta <[email protected]>

Signed-off-by: MyonKeminta <[email protected]>

lhy1024

the rest LGTM

lhy1024 · 2024-09-14T07:12:46Z

client/tso_stream.go

+		for !s.state.CompareAndSwap(streamStateIdle, streamStateClosing) {
+			switch state := s.state.Load(); state {
+			case streamStateIdle, streamStateSending:
+				// streamStateSending should switch to streamStateIdle very quickly. Spin until successfully setting to


Yes. Considering that there won't be any blocking operation in streamStateSending state and the state won't last long, I think spinning here is better than writing another syncing operation here.

ti-chi-bot · 2024-09-14T07:13:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JmPotato, lhy1024

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JmPotato,lhy1024]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-09-14T07:13:57Z

[LGTM Timeline notifier]

Timeline:

2024-09-14 07:03:22.710211923 +0000 UTC m=+685472.450635862: ☑️ agreed by JmPotato.
2024-09-14 07:13:56.838181014 +0000 UTC m=+686106.578604946: ☑️ agreed by lhy1024.

ti-chi-bot bot added the do-not-merge/needs-linked-issue label Aug 2, 2024

Add implementation of making tsoStream asynchronous

140a7c2

Signed-off-by: MyonKeminta <[email protected]>

MyonKeminta force-pushed the m/make-tso-stream-async branch from 881f542 to 140a7c2 Compare August 5, 2024 09:51

Add basic tests for rewritten tsoStream

88d9f5b

Signed-off-by: MyonKeminta <[email protected]>

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 7, 2024

MyonKeminta added 3 commits August 8, 2024 16:35

Add a concurrency test

623d896

Signed-off-by: MyonKeminta <[email protected]>

Fix some of CI failures

86d0f42

Signed-off-by: MyonKeminta <[email protected]>

Fix integration tests

ff0f11a

Signed-off-by: MyonKeminta <[email protected]>

ti-chi-bot bot removed the do-not-merge/needs-linked-issue label Aug 30, 2024

MyonKeminta added 5 commits September 2, 2024 16:38

Fix tso deadline

ef47b9d

Signed-off-by: MyonKeminta <[email protected]>

Fix stream broken handling

ff1d50d

Signed-off-by: MyonKeminta <[email protected]>

Fix comments

9076c9d

Signed-off-by: MyonKeminta <[email protected]>

try to avoid double invocation to the callback

39e4e0b

Signed-off-by: MyonKeminta <[email protected]>

Fix lint

632781b

Signed-off-by: MyonKeminta <[email protected]>

MyonKeminta marked this pull request as ready for review September 4, 2024 09:49

MyonKeminta changed the title ~~[WIP] client: Make tsoStream receives asynchronously.~~ client: Make tsoStream receives asynchronously. Sep 4, 2024

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2024

MyonKeminta changed the title ~~client: Make tsoStream receives asynchronously.~~ client: Make tsoStream receives asynchronously Sep 5, 2024

JmPotato reviewed Sep 6, 2024

View reviewed changes

client/tso_batch_controller.go Outdated Show resolved Hide resolved

client/tso_dispatcher.go Outdated Show resolved Hide resolved

client/tso_stream.go Show resolved Hide resolved

Address comments

8809c45

Signed-off-by: MyonKeminta <[email protected]>

lhy1024 reviewed Sep 9, 2024

View reviewed changes

MyonKeminta added 8 commits September 12, 2024 12:18

Merge branch 'm/add-benchmarks-for-tso-stream-and-dispatcher' into m/…

1362aac

…make-tso-stream-async

Fix build

4699b41

Signed-off-by: MyonKeminta <[email protected]>

Reimplement mock receiving

7dbcd77

Signed-off-by: MyonKeminta <[email protected]>

Merge branch 'm/add-benchmarks-for-tso-stream-and-dispatcher' into m/…

f4dc99a

…make-tso-stream-async

Fix disptacher benchmark

1f2dcd8

Signed-off-by: MyonKeminta <[email protected]>

Fix incorrect error generating

123a869

Signed-off-by: MyonKeminta <[email protected]>

Merge branch 'm/add-benchmarks-for-tso-stream-and-dispatcher' into m/…

2c6aaba

…make-tso-stream-async

Disable logs in BenchmarkTSOStreamSendRecv

bff9eae

Signed-off-by: MyonKeminta <[email protected]>

MyonKeminta added 4 commits September 12, 2024 16:14

Fix lint

ef6e365

Signed-off-by: MyonKeminta <[email protected]>

Merge branch 'm/add-benchmarks-for-tso-stream-and-dispatcher' into m/…

a9f4aee

…make-tso-stream-async

Make the comments more clear; unset req.streamID when getting new tso…

388d705

…Request from the pool Signed-off-by: MyonKeminta <[email protected]>

Fix lint

a183aca

Signed-off-by: MyonKeminta <[email protected]>

ti-chi-bot bot added do-not-merge/needs-linked-issue and removed do-not-merge/needs-linked-issue labels Sep 13, 2024

JmPotato approved these changes Sep 14, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Sep 14, 2024

lhy1024 approved these changes Sep 14, 2024

View reviewed changes

ti-chi-bot bot added the lgtm label Sep 14, 2024

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Sep 14, 2024

Merge branch 'master' into m/make-tso-stream-async

5314485

ti-chi-bot bot merged commit 71f6f96 into tikv:master Sep 14, 2024
25 checks passed

MyonKeminta mentioned this pull request Sep 14, 2024

client: Add benchmark for tsoStream and tsoDispatcher #8618

Closed

MyonKeminta deleted the m/make-tso-stream-async branch September 14, 2024 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: Make tsoStream receives asynchronously #8483

client: Make tsoStream receives asynchronously #8483

MyonKeminta commented Aug 2, 2024 •

edited

Loading

ti-chi-bot bot commented Aug 2, 2024

codecov bot commented Sep 4, 2024 •

edited

Loading

cfzjywxk commented Sep 6, 2024

lhy1024 Sep 9, 2024

MyonKeminta Sep 9, 2024

lhy1024 Sep 9, 2024

MyonKeminta Sep 9, 2024

lhy1024 Sep 9, 2024

MyonKeminta Sep 9, 2024

lhy1024 Sep 9, 2024

MyonKeminta Sep 9, 2024

MyonKeminta commented Sep 12, 2024 •

edited

Loading

lhy1024 left a comment

lhy1024 Sep 14, 2024

MyonKeminta Sep 14, 2024

ti-chi-bot bot commented Sep 14, 2024

ti-chi-bot bot commented Sep 14, 2024

		})

		// A large-enough capacity to hold maximum concurrent RPC requests. In our design, the concurrency is at most 16.

client: Make tsoStream receives asynchronously #8483

client: Make tsoStream receives asynchronously #8483

Conversation

MyonKeminta commented Aug 2, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Aug 2, 2024

codecov bot commented Sep 4, 2024 • edited Loading

Codecov Report

cfzjywxk commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MyonKeminta commented Sep 12, 2024 • edited Loading

lhy1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 14, 2024

ti-chi-bot bot commented Sep 14, 2024

[LGTM Timeline notifier]

MyonKeminta commented Aug 2, 2024 •

edited

Loading

codecov bot commented Sep 4, 2024 •

edited

Loading

MyonKeminta commented Sep 12, 2024 •

edited

Loading