statstics: reuse fmsketch by hawkingrei · Pull Request #47070 · pingcap/tidb

hawkingrei · 2023-09-18T14:55:47Z

What problem does this PR solve?

Issue Number: close #47071

Problem Summary:

What is changed and how it works?

Check List

Tests

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

before:

after:

No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

codecov · 2023-09-18T15:06:23Z

Codecov Report

Merging #47070 (179343e) into master (4450ae4) will decrease coverage by 0.3519%.
Report is 2 commits behind head on master.
The diff coverage is 100.0000%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #47070        +/-   ##
================================================
- Coverage   72.9633%   72.6115%   -0.3519%     
================================================
  Files          1337       1358        +21     
  Lines        399007     405580      +6573     
================================================
+ Hits         291129     294498      +3369     
- Misses        89075      92380      +3305     
+ Partials      18803      18702       -101

Flag	Coverage Δ
integration	`30.6066% <75.9259%> (?)`
unit	`72.9540% <100.0000%> (-0.0094%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9913% <ø> (ø)`
parser	`84.9620% <ø> (-0.0108%)`	⬇️
br	`48.5758% <ø> (-4.3639%)`	⬇️

time-and-fate

I think the logic in this PR is very fragile.
In this PR:

We put back the allFms in the pool in MergePartitionStats2GlobalStats
We put back the GlobalStats.Fms in the pool in handleGlobalStats and updateGlobalStats, which is returned from MergePartitionStats2GlobalStats
We put back the tableAllPartitionStats.Column.FMSketch in the pool in handleGlobalStats, which is also created in MergePartitionStats2GlobalStats.

The assumptions are very fragile, if we share structs in any two of the above three places, there will be bugs. And it's hard to realize this, because we put them back in the pool in totally different places.

statistics/fmsketch.go

time-and-fate · 2023-09-19T05:19:17Z

And we are handling the logic inconsistently.
When we call MergePartitionStats2GlobalStats from updateGlobalStats, we don't reuse the FM sketch in the allPartitionStats, but when we call MergePartitionStats2GlobalStats from handleGlobalStats, we reuse them.
Even in handleGlobalStats, we only reuse FM sketches from Column, but not reuse those from Index.

time-and-fate · 2023-09-19T06:31:10Z

My suggestion is to do all the Put() in MergePartitionStats2GlobalStats. And it looks like we can even remove Fms from the struct GlobalStats.

hawkingrei · 2023-09-19T10:42:57Z

I think the logic in this PR is very fragile. In this PR:
1. We put back the `allFms` in the pool in `MergePartitionStats2GlobalStats`

2. We put back the `GlobalStats.Fms` in the pool in `handleGlobalStats` and `updateGlobalStats`, which is returned from `MergePartitionStats2GlobalStats`

3. We put back the `tableAllPartitionStats.Column.FMSketch` in the pool in `handleGlobalStats`, which is also created in `MergePartitionStats2GlobalStats`.
The assumptions are very fragile, if we share structs in any two of the above three places, there will be bugs. And it's hard to realize this, because we put them back in the pool in totally different places.

In fact, we optimize the two places.

1、create FM sketch from the NewFmSketch
2、Create from the proto

all the implements create Fmsketch from the pool and release Fmsketch at the end of the lift.

the root cause is at the tableAllPartitionStats which the life of FMsketch is out of MergePartitionStats2GlobalStats.

hawkingrei · 2023-09-19T11:34:41Z

My suggestion is to do all the Put() in MergePartitionStats2GlobalStats. And it looks like we can even remove Fms from the struct GlobalStats.

I have refactor this code. Now we have an external cache in the mergeGlobalStatus. if it is not from the external, we will release it in the mergeGlobalStatus.

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

time-and-fate · 2023-09-19T13:22:42Z

statistics/handle/globalstats/global_stats.go

+	// initialized the globalStats
+	globalStats = new(GlobalStats)


Looks like we don't need this. We initiate globalStats below.

time-and-fate · 2023-09-19T13:24:40Z

statistics/table.go

+// ReleaseAndPutToPool releases data structures of Table and put itself back to pool.
+func (t *Table) ReleaseAndPutToPool() {
+	for _, col := range t.Columns {
+		col.FMSketch.DestroyAndPutToPool()


I think it's safer to set FMSketch to nil after putting to the pool.

Ok to me to improve them in another PR or in another way at your convenience.

Done，use map.clear to clean it.

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

ti-chi-bot · 2023-09-19T13:37:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qw4990, time-and-fate

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [qw4990,time-and-fate]

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 · 2023-09-19T13:37:17Z

[LGTM Timeline notifier]

Timeline:

2023-09-19 02:05:06.020853202 +0000 UTC m=+569471.988441252: ☑️ agreed by qw4990.
2023-09-19 13:37:16.094453453 +0000 UTC m=+611002.062041506: ☑️ agreed by time-and-fate.

tiprow · 2023-09-19T13:37:55Z

@hawkingrei: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
tiprow_fast_test	`179343e`	link	true	`/test tiprow_fast_test`

Full PR test history. Your PR dashboard.

Details

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 kubernetes/test-infra repository. I understand the commands that are listed here.

hawkingrei · 2023-09-19T14:52:13Z

/retest

hawkingrei · 2023-12-19T02:42:07Z

/cherrypick release-6.5

ti-chi-bot · 2023-12-19T02:42:50Z

@hawkingrei: new pull request created to branch release-6.5: #49573.

Details

In response to this:

/cherrypick release-6.5

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.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

close #47071

…anic When FMSketchFromProto and NewFMSketch retrieve an FMSketch from sync.Pool, they must call reset() to clear stale hashset state. Without this, a reused swiss.Map can have inconsistent internal metadata, causing an index-out-of-range panic in swiss.Map.Put. The bug was introduced in bb49dc1 ("statstics: reuse fmsketch", pingcap#47070) which added pool reuse but omitted the reset call. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

hawkingrei requested review from 0xPoe and qw4990 September 19, 2023 01:54

qw4990 approved these changes Sep 19, 2023

View reviewed changes

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

hawkingrei requested review from time-and-fate and winoros September 19, 2023 02:52

hawkingrei force-pushed the reuse_fmsketch branch from d704319 to 882572e Compare September 19, 2023 04:00

time-and-fate reviewed Sep 19, 2023

View reviewed changes

statistics/fmsketch.go Outdated Show resolved Hide resolved

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 19, 2023

hawkingrei added 8 commits September 19, 2023 19:35

statstics: reuse fmsketch

1c5cc5a

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

statstics: reuse fmsketch

19948df

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

statstics: reuse fmsketch

b48b97e

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

statstics: reuse fmsketch

770b42f

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

statstics: reuse fmsketch

b43e238

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

b97ea0e

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

3431d52

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

4f718d4

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

hawkingrei force-pushed the reuse_fmsketch branch from 2d79e44 to 4f718d4 Compare September 19, 2023 11:35

hawkingrei added 3 commits September 19, 2023 19:42

update

cbea358

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

7a357d6

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

aba02a4

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

time-and-fate reviewed Sep 19, 2023

View reviewed changes

update

179343e

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

time-and-fate approved these changes Sep 19, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 19, 2023

ti-chi-bot bot merged commit bb49dc1 into pingcap:master Sep 19, 2023

ti-chi-bot mentioned this pull request Dec 19, 2023

statstics: reuse fmsketch (#47070) #49573

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Dec 19, 2023

This is an automated cherry-pick of pingcap#47070

cfa9239

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot bot pushed a commit that referenced this pull request Dec 19, 2023

statstics: reuse fmsketch (#47070) (#49573)

ba5d946

close #47071

mjonss mentioned this pull request Feb 28, 2026

statistics: simplify FMSketch by removing swiss map and pool #66590

Merged

13 tasks

		// initialized the globalStats
		globalStats = new(GlobalStats)

Conversation

hawkingrei commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

codecov bot commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

time-and-fate left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

time-and-fate commented Sep 19, 2023

Uh oh!

time-and-fate commented Sep 19, 2023

Uh oh!

hawkingrei commented Sep 19, 2023

Uh oh!

hawkingrei commented Sep 19, 2023

Uh oh!

time-and-fate Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

hawkingrei Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

time-and-fate Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

time-and-fate Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

hawkingrei Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Sep 19, 2023

Uh oh!

ti-chi-bot bot commented Sep 19, 2023

[LGTM Timeline notifier]

Uh oh!

tiprow bot commented Sep 19, 2023

Uh oh!

hawkingrei commented Sep 19, 2023

Uh oh!

hawkingrei commented Dec 19, 2023

Uh oh!

ti-chi-bot commented Dec 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hawkingrei commented Sep 18, 2023 •

edited

Loading

codecov bot commented Sep 18, 2023 •

edited

Loading

time-and-fate left a comment •

edited

Loading