statistics: use another way to merge topn #47765

winoros · 2023-10-18T19:48:31Z

What problem does this PR solve?

Issue Number: ref #50761

Problem Summary:

What is changed and how it works?

We use the property that items inside both the TopN and the histogram are ordered to speed up the process.

And use a heuristic cutting:

We record the avg num per distinct value of each histogram notNullCount / ndv_in_hist.
Then for a topn item, we can use the mentioned property to know the TopNs that contain the item.
Then we can easily know in which partition the item occurs in the histogram.
The maximum possible occurrence of this item is the sum occurrence of the affected TopNs + the avg num per distinct value of each affected histogram
If the maximum possible occurrence is still smaller than the currently maintained smallest global TopN item, we don't need to insert this one into the heap and test it.

In this way, the speed is improved while the CPU is saved.

The CPU usage. Previous VS This pull.

Running tool: /DATA/disk4/yiding/go/bin/go test -benchmem -run=^$ -tags intest,deadlock -bench ^BenchmarkMergePartTopN2GlobalTopNWithHists$ github.com/pingcap/tidb/pkg/statistics/handle/globalstats

goos: linux
goarch: amd64
pkg: github.com/pingcap/tidb/pkg/statistics/handle/globalstats
cpu: Intel(R) Xeon(R) Gold 6240 CPU @ 2.60GHz
BenchmarkMergePartTopN2GlobalTopNWithHists/Size100-72         	     159	   7249785 ns/op	   34714 B/op	     113 allocs/op
BenchmarkMergePartTopN2GlobalTopNWithHists/Size1000-72        	       9	 117386869 ns/op	  100136 B/op	    1013 allocs/op
BenchmarkMergePartTopN2GlobalTopNWithHists/Size2000-72        	       4	 272247218 ns/op	  173416 B/op	    2013 allocs/op
BenchmarkMergePartTopN2GlobalTopNWithHists/Size5000-72        	       2	 787035728 ns/op	  393256 B/op	    5013 allocs/op
BenchmarkMergePartTopN2GlobalTopNWithHists/Size10000-72       	       1	1919716271 ns/op	  759656 B/op	   10013 allocs/op
PASS
ok  	github.com/pingcap/tidb/pkg/statistics/handle/globalstats	17.839s

Running tool: /DATA/disk4/yiding/go/bin/go test -benchmem -run=^$ -tags intest,deadlock -bench ^BenchmarkMergePartTopN2GlobalTopNWithHists$ github.com/pingcap/tidb/pkg/statistics/handle/globalstats

goos: linux
goarch: amd64
pkg: github.com/pingcap/tidb/pkg/statistics/handle/globalstats
cpu: Intel(R) Xeon(R) Gold 6240 CPU @ 2.60GHz
BenchmarkMergePartTopN2GlobalTopNWithHists/Size100-72         	     122	  10071203 ns/op	  177982 B/op	      30 allocs/op
BenchmarkMergePartTopN2GlobalTopNWithHists/Size1000-72        	       6	 186292872 ns/op	  176418 B/op	      28 allocs/op
BenchmarkMergePartTopN2GlobalTopNWithHists/Size2000-72        	       3	 398591599 ns/op	  178269 B/op	      31 allocs/op
BenchmarkMergePartTopN2GlobalTopNWithHists/Size5000-72        	       1	1436609512 ns/op	  179432 B/op	      33 allocs/op
BenchmarkMergePartTopN2GlobalTopNWithHists/Size10000-72       	       1	4363627038 ns/op	  177336 B/op	      31 allocs/op
PASS
ok  	github.com/pingcap/tidb/pkg/statistics/handle/globalstats	18.587s

n=100,            7249785/10071203=72%
n=1000         117386869/186292872=63%
n=2000         272247218/398591599=68%
n=5000        787035728/1436609512=55%
n=10000      1919716271/4363627038=44%

The mem usage is also in an acceptable range.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
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

ti-chi-bot · 2023-11-17T15:54:36Z

PR needs rebase.

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.

codecov · 2024-01-26T17:40:43Z

Codecov Report

Attention: Patch coverage is 82.51121% with 39 lines in your changes missing coverage. Please review.

Project coverage is 73.6702%. Comparing base (41c3b01) to head (4efdb94).
Report is 10 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #47765        +/-   ##
================================================
+ Coverage   73.0683%   73.6702%   +0.6018%     
================================================
  Files          1687       1719        +32     
  Lines        466567     476392      +9825     
================================================
+ Hits         340913     350959     +10046     
+ Misses       104711     103715       -996     
- Partials      20943      21718       +775

Flag	Coverage Δ
integration	`45.6218% <70.8520%> (?)`
unit	`72.2819% <82.5112%> (+0.0216%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`44.7575% <ø> (-0.5773%)`	⬇️

pkg/statistics/handle/globalstats/global_stats.go

winoros · 2024-01-29T13:08:22Z

And some codes like the in-place updates for the heap are optimized for the memory.

winoros · 2024-02-05T22:11:37Z

/retest

pkg/statistics/handle/globalstats/topn.go

elsa0520 · 2024-02-06T09:42:52Z

pkg/statistics/handle/globalstats/topn.go

+	globalTopN.Sort()
+	return &globalTopN, remainedTopNs, hists, nil
+}
+


Mark as deprecated

pkg/statistics/handle/globalstats/global_stats_test.go

hawkingrei · 2024-02-20T07:45:46Z

/retest

Rustin170506

Just a quick run through the code, I'm still trying to understand the business logic of it. I'll look at it a couple more times soon to try and understand it.

pkg/statistics/cmsketch_util.go

pkg/statistics/handle/globalstats/global_stats_test.go

pkg/statistics/handle/globalstats/topn.go

Rustin170506

Looks good to me. But please move checkTheCurAndMoveForward out as a wrapper and add more comments. Thank you for working on this. Approve in advance.

Rustin170506 · 2024-02-22T09:49:19Z

pkg/statistics/handle/globalstats/topn.go

+			affectedHist = append(affectedHist, int(histPos))
+		}
+		// Hacking skip.
+		if uint32(len(finalTopNs)) >= n {


I am not sure which one we prefer. len(finalTopNs) vs. finalTopNs.Len(). But I guess it doesn't matter.

pkg/statistics/handle/globalstats/topn.go

ti-chi-bot · 2024-02-22T09:58:29Z

[LGTM Timeline notifier]

Timeline:

2024-02-22 09:58:29.271545344 +0000 UTC m=+523998.019168506: ☑️ agreed by hi-rustin.

hawkingrei · 2024-06-09T13:31:33Z

/test all

ti-chi-bot · 2024-09-28T18:33:49Z

PR needs rebase.

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-sigs/prow repository.

ti-chi-bot · 2025-01-24T08:49:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Rustin170506

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:

~~pkg/statistics/OWNERS~~ [Rustin170506]

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

winoros added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2023

ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 18, 2023

winoros force-pushed the merge-topn-one-pass branch from dfddf2e to 54fc562 Compare October 18, 2023 20:18

statistics: use another way to merge topn

e0e2588

winoros force-pushed the merge-topn-one-pass branch from 54fc562 to e0e2588 Compare October 18, 2023 21:34

winoros added 2 commits October 19, 2023 05:36

Merge branch 'master' into merge-topn-one-pass

99327a9

fantastic skipping

72adddd

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 18, 2023

winoros added 3 commits October 19, 2023 07:15

use Fix instead of Pop then Push

0086a8c

fix topn num and enlarge the log's step

abf47b8

save some cpu for calling NextClear

547f9c6

winoros force-pushed the merge-topn-one-pass branch from 702a07f to 547f9c6 Compare October 19, 2023 19:02

winoros added 2 commits October 20, 2023 04:06

change log pos and fix deadloop

b697bbf

Merge branch 'master' into merge-topn-one-pass

26fd8da

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

Merge branch 'master' into merge-topn-one-pass

8fa85f9

ti-chi-bot bot removed do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked labels Jan 26, 2024

winoros added sig/planner SIG: Planner and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 26, 2024

winoros commented Jan 26, 2024

View reviewed changes

pkg/statistics/handle/globalstats/global_stats.go Outdated Show resolved Hide resolved

hawkingrei self-requested a review January 28, 2024 13:54

Rustin170506 self-requested a review February 1, 2024 02:07

winoros added 6 commits February 6, 2024 04:58

fix the bug of codes

6c01400

redo the num change to reduce the diff

78f35ea

skip two test

6a61462

fix linting

fd09e6f

remove dead codes

1dc2a28

Merge branch 'master' into merge-topn-one-pass

06f4d41

elsa0520 reviewed Feb 6, 2024

View reviewed changes

pkg/statistics/handle/globalstats/global_stats_test.go Outdated Show resolved Hide resolved

Rustin170506 reviewed Feb 20, 2024

View reviewed changes

winoros added 2 commits February 21, 2024 17:31

Merge remote-tracking branch 'origin/master' into merge-topn-one-pass

6dcbcc0

address comments

6f0c669

Rustin170506 approved these changes Feb 22, 2024

View reviewed changes

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

winoros added 3 commits February 26, 2024 00:00

use a wrapper

8038ee3

Merge branch 'master' into merge-topn-one-pass

b819a09

Merge branch 'master' into merge-topn-one-pass

606519c

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

winoros added 4 commits January 22, 2025 18:05

Merge branch 'master' into merge-topn-one-pass

3d4997e

Merge remote-tracking branch 'origin/master' into merge-topn-one-pass

63a02bb

Merge remote-tracking branch 'origin/master' into merge-topn-one-pass

78a3d37

unskip tests

4efdb94

winoros removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statistics: use another way to merge topn #47765

statistics: use another way to merge topn #47765

winoros commented Oct 18, 2023 •

edited

Loading

ti-chi-bot bot commented Nov 17, 2023

codecov bot commented Jan 26, 2024 •

edited

Loading

winoros commented Jan 29, 2024

winoros commented Feb 5, 2024

elsa0520 Feb 6, 2024

hawkingrei commented Feb 20, 2024

Rustin170506 left a comment

Rustin170506 left a comment

Rustin170506 Feb 22, 2024

ti-chi-bot bot commented Feb 22, 2024

hawkingrei commented Jun 9, 2024

ti-chi-bot bot commented Sep 28, 2024

ti-chi-bot bot commented Jan 24, 2025

statistics: use another way to merge topn #47765

Are you sure you want to change the base?

statistics: use another way to merge topn #47765

Conversation

winoros commented Oct 18, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot bot commented Nov 17, 2023

codecov bot commented Jan 26, 2024 • edited Loading

Codecov Report

winoros commented Jan 29, 2024

winoros commented Feb 5, 2024

elsa0520 Feb 6, 2024

Choose a reason for hiding this comment

hawkingrei commented Feb 20, 2024

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 Feb 22, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Feb 22, 2024

[LGTM Timeline notifier]

hawkingrei commented Jun 9, 2024

ti-chi-bot bot commented Sep 28, 2024

ti-chi-bot bot commented Jan 24, 2025

winoros commented Oct 18, 2023 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading