core: separate region lock #5720

rleungx · 2022-11-21T10:49:09Z

Signed-off-by: Ryan Leung [email protected]

What problem does this PR solve?

Issue Number: Ref #5712, #5586, #5648.

What is changed and how does it work?

master:

[2022/11/23 15:13:15.599 +08:00] [INFO] [main.go:452] ["update speed of each category"] [rps=56747.8917] [save-tree=3404.8735] [save-kv=2269.9157] [save-space=8512.1838] [save-flow=19861.7621] [skip=28286.8378]
[2022/11/23 15:13:15.706 +08:00] [INFO] [main.go:398] ["region heartbeat stats"] [total=35.2433s] [slowest=34.8643s] [fastest=0.0000s] [average=16.5034s] [stddev=10.3169s] [rps=56748.3756]

this branch:

[2022/11/23 15:20:38.078 +08:00] [INFO] [main.go:452] ["update speed of each category"] [rps=80912.1055] [save-tree=4854.7263] [save-kv=3236.4842] [save-space=12136.8158] [save-flow=28319.2369] [skip=40331.8527]
[2022/11/23 15:20:38.239 +08:00] [INFO] [main.go:398] ["region heartbeat stats"] [total=24.7180s] [slowest=23.8633s] [fastest=0.0000s] [average=11.1023s] [stddev=6.8950s] [rps=80912.7660]

Check List

Tests

Unit test

Release note

None

ti-chi-bot · 2022-11-21T10:49:11Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

CabinfeverB
HunDunDM

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

codecov · 2022-11-21T11:00:42Z

Codecov Report

Base: 75.58% // Head: 75.60% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (236c6dd) compared to base (b94d940).
Patch coverage: 86.66% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5720      +/-   ##
==========================================
+ Coverage   75.58%   75.60%   +0.01%     
==========================================
  Files         330      330              
  Lines       32895    32927      +32     
==========================================
+ Hits        24864    24894      +30     
- Misses       5880     5885       +5     
+ Partials     2151     2148       -3

Flag	Coverage Δ
unittests	`75.60% <86.66%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
server/core/region.go	`89.90% <85.54%> (-2.31%)`	⬇️
server/core/basic_cluster.go	`90.81% <92.85%> (+0.49%)`	⬆️
pkg/mock/mockcluster/mockcluster.go	`93.94% <100.00%> (-0.02%)`	⬇️
server/schedule/range_cluster.go	`83.63% <100.00%> (+0.30%)`	⬆️
server/api/diagnostic.go	`76.92% <0.00%> (-23.08%)`	⬇️
pkg/tempurl/tempurl.go	`60.00% <0.00%> (-10.00%)`	⬇️
server/tso/tso.go	`68.53% <0.00%> (-7.31%)`	⬇️
server/cluster/diagnostic_manager.go	`72.04% <0.00%> (-4.31%)`	⬇️
server/id/id.go	`86.44% <0.00%> (-3.39%)`	⬇️
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

CabinfeverB

we can remove RWMutex in Regions

HunDunDM · 2022-11-24T07:42:10Z

tests/server/cluster/cluster_test.go

@@ -565,6 +566,7 @@ func TestStoreVersionChange(t *testing.T) {

 func TestConcurrentHandleRegion(t *testing.T) {
 	re := require.New(t)
+	deadlock.Opts.DeadlockTimeout = time.Minute


The default value is 30s, which will report potential deadlock when running the concurrent region heartbeat test since the deadlock detection will cost much time.

HunDunDM · 2022-11-24T07:44:25Z

server/cluster/cluster_test.go

 		checkRegion(re, cache.GetRegion(i), newRegion)
 		checkRegion(re, cache.GetRegionByKey(regionKey), newRegion)
 		checkRegions(re, cache, regions[0:(i+1)])

 		cache.RemoveRegion(region)
+		cache.RemoveRegionFromSubTree(region)


Why split it?

It is intended to split region lock into region tree lock and subtree lock.

HunDunDM · 2022-11-24T07:58:00Z

server/core/basic_cluster.go

-		mu syncutil.RWMutex
-		*RegionsInfo
-	}
+	Regions *RegionsInfo


How about *RegionsInfo? It can reduce the repeated definition of many functions.

HunDunDM · 2022-11-24T08:02:14Z

server/core/basic_cluster.go

 	bc.Regions.RemoveRegion(region)
+	bc.Regions.RemoveRegionFromSubTree(region)


Is it possible for temporary Tree and SubTree inconsistencies to introduce risks?

What kind of risk? I think subtree is used for scheduling mostly.

HunDunDM · 2022-11-24T08:02:42Z

server/schedule/range_cluster.go

@@ -31,7 +31,8 @@ type RangeCluster struct {
 func GenRangeCluster(cluster Cluster, startKey, endKey []byte) *RangeCluster {
 	subCluster := core.NewBasicCluster()
 	for _, r := range cluster.ScanRegions(startKey, endKey, -1) {
-		subCluster.Regions.SetRegion(r)
+		origin, _, toRemove, rangeChanged := subCluster.Regions.SetRegionWithUpdate(r)
+		subCluster.Regions.UpdateSubTree(r, origin, toRemove, rangeChanged)


HunDunDM

rest LGTM

HunDunDM · 2022-11-29T06:39:52Z

server/core/region.go

+	t            syncutil.RWMutex
 	tree         *regionTree
 	regions      map[uint64]*regionItem // regionID -> regionInfo
+	st           syncutil.RWMutex
+	subRegions   map[uint64]*regionItem // regionID -> regionInfo


Use code like this to identify the scope:

regionTree struct { syncutil.RWMutex *regionTree }

The map also needs to be locked, so maybe wrap them later?

Signed-off-by: Ryan Leung <[email protected]>

rleungx · 2022-11-30T10:06:45Z

/hold

rleungx · 2022-11-30T10:21:16Z

/hold cancel

rleungx · 2022-11-30T11:12:40Z

/merge

ti-chi-bot · 2022-11-30T11:12:41Z

@rleungx: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-11-30T11:12:42Z

This pull request has been accepted and is ready to merge.

Commit hash: 9931950

ti-chi-bot · 2022-11-30T11:12:56Z

@rleungx: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot added do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 21, 2022

ti-chi-bot requested review from lhy1024 and Yisaer November 21, 2022 10:49

rleungx force-pushed the split-lock branch from e2af7e3 to aa6a8d9 Compare November 23, 2022 06:43

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 23, 2022

rleungx marked this pull request as ready for review November 23, 2022 06:55

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

rleungx requested review from HunDunDM, bufferflies, CabinfeverB and nolouch and removed request for Yisaer and lhy1024 November 23, 2022 06:56

CabinfeverB reviewed Nov 24, 2022

View reviewed changes

rleungx force-pushed the split-lock branch from f56f0fa to d31ae5c Compare November 24, 2022 06:02

HunDunDM reviewed Nov 24, 2022

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2022

rleungx force-pushed the split-lock branch from d7686d6 to 147da5c Compare November 24, 2022 08:50

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2022

rleungx requested review from HunDunDM and CabinfeverB November 29, 2022 06:00

HunDunDM approved these changes Nov 29, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 29, 2022

separate region lock

a4aaf40

Signed-off-by: Ryan Leung <[email protected]>

rleungx added 3 commits November 30, 2022 13:13

skip update tree item when the range isn't changed

a62db30

Signed-off-by: Ryan Leung <[email protected]>

address the comment

4a13672

Signed-off-by: Ryan Leung <[email protected]>

address the comment

c7d4b45

Signed-off-by: Ryan Leung <[email protected]>

rleungx force-pushed the split-lock branch from 147da5c to c7d4b45 Compare November 30, 2022 05:18

address the comment

9931950

Signed-off-by: Ryan Leung <[email protected]>

CabinfeverB approved these changes Nov 30, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 30, 2022

rleungx mentioned this pull request Nov 30, 2022

Improve the performance of handling region heartbeats #5648

Closed

16 tasks

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2022

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 30, 2022

Merge branch 'master' into split-lock

236c6dd

ti-chi-bot merged commit 58fde49 into tikv:master Nov 30, 2022

rleungx deleted the split-lock branch June 9, 2023 05:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: separate region lock #5720

core: separate region lock #5720

rleungx commented Nov 21, 2022 •

edited

Loading

ti-chi-bot commented Nov 21, 2022 •

edited

Loading

codecov bot commented Nov 21, 2022 •

edited

Loading

CabinfeverB left a comment

HunDunDM Nov 24, 2022

rleungx Nov 24, 2022

HunDunDM Nov 24, 2022

rleungx Nov 24, 2022

HunDunDM Nov 24, 2022

rleungx Nov 24, 2022

HunDunDM Nov 24, 2022

rleungx Nov 24, 2022

HunDunDM Nov 24, 2022

HunDunDM left a comment

HunDunDM Nov 29, 2022 •

edited

Loading

rleungx Nov 30, 2022

rleungx commented Nov 30, 2022

rleungx commented Nov 30, 2022

rleungx commented Nov 30, 2022

ti-chi-bot commented Nov 30, 2022

ti-chi-bot commented Nov 30, 2022

ti-chi-bot commented Nov 30, 2022

		bc.Regions.RemoveRegion(region)
		bc.Regions.RemoveRegionFromSubTree(region)

core: separate region lock #5720

core: separate region lock #5720

Conversation

rleungx commented Nov 21, 2022 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Nov 21, 2022 • edited Loading

codecov bot commented Nov 21, 2022 • edited Loading

Codecov Report

CabinfeverB left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HunDunDM left a comment

Choose a reason for hiding this comment

HunDunDM Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx commented Nov 30, 2022

rleungx commented Nov 30, 2022

rleungx commented Nov 30, 2022

ti-chi-bot commented Nov 30, 2022

ti-chi-bot commented Nov 30, 2022

ti-chi-bot commented Nov 30, 2022

rleungx commented Nov 21, 2022 •

edited

Loading

ti-chi-bot commented Nov 21, 2022 •

edited

Loading

codecov bot commented Nov 21, 2022 •

edited

Loading

HunDunDM Nov 29, 2022 •

edited

Loading