cdc: replace BtreeMap with HashMap in TiCDC #8021

overvenus · 2023-01-05T10:31:41Z

What problem does this PR solve?

Issue Number: ref #7720

What is changed and how it works?

Replace HashMap with BtreeMap in TiCDC, benchmark show hash map is faster in get, insert, and delete, but is slower in range.

cpu: Intel(R) Xeon(R) CPU E5-2630 v4 @ 2.20GHz
BenchmarkMap/Get/BtreeMap-40             3258027               398.7 ns/op             0 B/op          0 allocs/op
BenchmarkMap/Get/HashMap-40             11326316               108.1 ns/op             0 B/op          0 allocs/op
BenchmarkMap/Get/SyncMap-40              8013238               141.1 ns/op             0 B/op          0 allocs/op
BenchmarkMap/Store/BtreeMap-40           3021882               434.3 ns/op             0 B/op          0 allocs/op
BenchmarkMap/Store/HashMap-40            8651628               128.3 ns/op             0 B/op          0 allocs/op
BenchmarkMap/Store/SyncMap-40            3344292               412.5 ns/op            71 B/op          2 allocs/op
BenchmarkMap/Delete+Store/BtreeMap-40            1334109               909.3 ns/op             0 B/op          0 allocs/op
BenchmarkMap/Delete+Store/HashMap-40             4777480               239.9 ns/op             0 B/op          0 allocs/op
BenchmarkMap/Delete+Store/SyncMap-40             2168637               561.2 ns/op            71 B/op          2 allocs/op
BenchmarkMap/Range/BtreeMap-40                    825788              1420 ns/op               0 B/op          0 allocs/op
BenchmarkMap/Range/HashMap-40                     210938              4780 ns/op               0 B/op          0 allocs/op
BenchmarkMap/Range/SyncMap-40                     241045              6008 ns/op               0 B/op          0 allocs/op
BenchmarkMap/AscendRange/BtreeMap-40              288669              3933 ns/op               0 B/op          0 allocs/op

Check List

Tests

Unit test
Integration test

Questions

Will it cause performance regression or break compatibility?

No

Do you need to update user documentation, design documentation or monitoring documentation?

No

Release note

None

ti-chi-bot · 2023-01-05T10:31:43Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

3AceShowHand
sdojjy

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.

Signed-off-by: Neil Shen <overvenus@gmail.com>

CharlesCheung96 · 2023-01-06T14:12:59Z

Please explain in the pr body why you made this substitution.

CharlesCheung96 · 2023-01-06T14:15:22Z

cdc/processor/manager_test.go

@@ -305,7 +305,7 @@ func TestQueryTableCount(t *testing.T) {
 	m := NewManager(&model.CaptureInfo{ID: "capture-test"}, nil, &liveness, cfg).(*managerImpl)
 	ctx := context.TODO()
 	// Add some tables to processor.
-	tables := spanz.NewMap[tablepb.TablePipeline]()
+	tables := spanz.NewHashMap[tablepb.TablePipeline]()


What is the meaning of the package name spanz?

It should be spans (just like the strings package), but it may cause naming confliction, there are so many spans variables. So the name spanz, easy to type and easy to pronounce.

Signed-off-by: Neil Shen <overvenus@gmail.com>

3AceShowHand · 2023-01-10T06:42:08Z

cdc/processor/sinkmanager/redo_cache.go

@@ -132,7 +132,7 @@ func (r *redoEventCache) maybeCreateAppender(
 func (r *redoEventCache) getAppender(span tablepb.Span) *eventAppender {
 	r.mu.Lock()
 	defer r.mu.Unlock()
-	return r.tables[spanz.ToHashableSpan(span)]
+	return r.tables.GetV(span)


Why define such a method GetV, I think only Get is needed.

sdojjy · 2023-01-11T06:58:54Z

/merge

ti-chi-bot · 2023-01-11T06:58:56Z

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

Commit hash: 0d1840b

overvenus · 2023-01-11T10:22:26Z

/run-engine-integration-test
/run-verify

overvenus · 2023-01-12T07:15:32Z

/run-integration-test

overvenus · 2023-01-12T08:44:37Z

/run-engine-integration-test
/run-dm-integration-test

Signed-off-by: Neil Shen <overvenus@gmail.com>

overvenus · 2023-01-12T09:21:28Z

/merge

ti-chi-bot · 2023-01-12T09:21:31Z

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

Commit hash: 5df46fe

Signed-off-by: Neil Shen <overvenus@gmail.com>

overvenus · 2023-01-12T10:08:33Z

/merge

ti-chi-bot · 2023-01-12T10:08:36Z

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

Commit hash: 004a28b

overvenus · 2023-01-12T10:38:09Z

/run-engine-integration-test

ti-chi-bot · 2023-01-12T10:44:03Z

@overvenus: 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

trigger some heavy tests which will not run always when PR updated.

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.

overvenus added the area/ticdc Issues or PRs related to TiCDC. label Jan 5, 2023

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 5, 2023

overvenus mentioned this pull request Jan 5, 2023

Tracking issue for scale-out single large table to multiple TiCDC instances #7720

Closed

22 tasks

overvenus changed the title ~~cdc: replace HashMap with BtreeMap in TiCDC~~ cdc: replace BtreeMap with HashMap in TiCDC Jan 6, 2023

overvenus added 3 commits January 6, 2023 15:47

cdc: replace BtreeMap with HashMap in TiCDC

1911312

Signed-off-by: Neil Shen <overvenus@gmail.com>

scheduler,spanz(ticdc): specify BtreeMap degree

0c8f16e

Signed-off-by: Neil Shen <overvenus@gmail.com>

spanz(ticdc): reduce hashableSpan memory allocation

471f173

Signed-off-by: Neil Shen <overvenus@gmail.com>

overvenus force-pushed the span-replication/maps branch from 1762a15 to 471f173 Compare January 6, 2023 07:48

overvenus requested review from sdojjy, zhaoxinyu and 3AceShowHand January 6, 2023 07:49

overvenus marked this pull request as ready for review January 6, 2023 07:49

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 Jan 6, 2023

CharlesCheung96 reviewed Jan 6, 2023

View reviewed changes

overvenus added 2 commits January 9, 2023 14:38

add span map benchmark

183ef97

Signed-off-by: Neil Shen <overvenus@gmail.com>

address lints

0d1840b

Signed-off-by: Neil Shen <overvenus@gmail.com>

3AceShowHand reviewed Jan 10, 2023

View reviewed changes

3AceShowHand approved these changes Jan 10, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 10, 2023

CharlesCheung96 approved these changes Jan 11, 2023

View reviewed changes

sdojjy approved these changes Jan 11, 2023

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 Jan 11, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 11, 2023

Merge branch 'master' into span-replication/maps

996f0a0

ti-chi-bot added 4 commits January 11, 2023 23:48

Merge branch 'master' into span-replication/maps

ed49771

Merge branch 'master' into span-replication/maps

9bb71f0

Merge branch 'master' into span-replication/maps

0141d06

Merge branch 'master' into span-replication/maps

ec7cbad

Merge branch 'master' into span-replication/maps

6b765be

Merge branch 'master' into span-replication/maps

5df46fe

Signed-off-by: Neil Shen <overvenus@gmail.com>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Jan 12, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 12, 2023

ti-chi-bot and others added 2 commits January 12, 2023 17:24

Merge branch 'master' into span-replication/maps

8d4c130

stable test

004a28b

Signed-off-by: Neil Shen <overvenus@gmail.com>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Jan 12, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 12, 2023

Merge branch 'master' into span-replication/maps

c58a7f6

ti-chi-bot merged commit ad4dc4a into pingcap:master Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cdc: replace BtreeMap with HashMap in TiCDC #8021

cdc: replace BtreeMap with HashMap in TiCDC #8021

overvenus commented Jan 5, 2023 •

edited

Loading

ti-chi-bot commented Jan 5, 2023 •

edited

Loading

CharlesCheung96 commented Jan 6, 2023

CharlesCheung96 Jan 6, 2023

overvenus Jan 9, 2023

3AceShowHand Jan 10, 2023

sdojjy commented Jan 11, 2023

ti-chi-bot commented Jan 11, 2023

overvenus commented Jan 11, 2023

overvenus commented Jan 12, 2023

overvenus commented Jan 12, 2023

overvenus commented Jan 12, 2023

ti-chi-bot commented Jan 12, 2023

overvenus commented Jan 12, 2023

ti-chi-bot commented Jan 12, 2023

overvenus commented Jan 12, 2023

ti-chi-bot commented Jan 12, 2023

cdc: replace BtreeMap with HashMap in TiCDC #8021

cdc: replace BtreeMap with HashMap in TiCDC #8021

Conversation

overvenus commented Jan 5, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Tests

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

ti-chi-bot commented Jan 5, 2023 • edited Loading

CharlesCheung96 commented Jan 6, 2023

CharlesCheung96 Jan 6, 2023

Choose a reason for hiding this comment

overvenus Jan 9, 2023

Choose a reason for hiding this comment

3AceShowHand Jan 10, 2023

Choose a reason for hiding this comment

sdojjy commented Jan 11, 2023

ti-chi-bot commented Jan 11, 2023

overvenus commented Jan 11, 2023

overvenus commented Jan 12, 2023

overvenus commented Jan 12, 2023

overvenus commented Jan 12, 2023

ti-chi-bot commented Jan 12, 2023

overvenus commented Jan 12, 2023

ti-chi-bot commented Jan 12, 2023

overvenus commented Jan 12, 2023

ti-chi-bot commented Jan 12, 2023

overvenus commented Jan 5, 2023 •

edited

Loading

ti-chi-bot commented Jan 5, 2023 •

edited

Loading