statistics: add lru cache implement for statsCacheInner #34145

Yisaer · 2022-04-21T05:28:55Z

What problem does this PR solve?

Issue Number: ref #34052

Problem Summary:

What is changed and how it works?

add lru cache implement for statsCacheInner.

The evict policy is desribed in this document

Check List

Tests

Unit test

Release note

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

None

ti-chi-bot · 2022-04-21T05:28:56Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

sre-bot · 2022-04-21T05:38:40Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/1341eb0303e21d8626e310790578d1cfe9b224ec

Signed-off-by: yisaer <[email protected]> add lru statscache Signed-off-by: yisaer <[email protected]> add lru statscache Signed-off-by: yisaer <[email protected]>

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

statistics/handle/lru_cache.go

metrics/stats.go

chrysan · 2022-04-22T12:31:34Z

statistics/handle/lru_cache.go

+	if !exists {
+		return nil, false
+	}
+	l.cache.MoveToFront(element)


Should we maintain the lru list for Column instead of Table?

I think we can't, because the basic recent used element is table for current code logic.

eg: For now we can only know the updated table id in following code, thus it's hard to maintain column in lru instead of table.

// update updates the statistics table cache using copy on write. func (sc statsCache) update(tables []*statistics.Table, deletedIDs []int64, newVersion uint64) statsCache { newCache := sc.copy() if newVersion == newCache.version { newCache.minorVersion += uint64(1) } else { newCache.version = newVersion newCache.minorVersion = uint64(0) } for _, tbl := range tables { id := tbl.PhysicalID newCache.Put(id, tbl) } for _, id := range deletedIDs { newCache.Del(id) } return newCache }

Agree, the information about which columns are used after getTableStats is everywhere. We can leverage CollectColumnStatsUsage later to maintain LRU of columns.

And should only MoveToFront for GetByQuery.

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

statistics/handle/lru_cache.go

statistics/handle/statscache.go

statistics/handle/lru_cache.go

statistics/handle/lru_cache_test.go

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

Signed-off-by: yisaer <[email protected]> fix test Signed-off-by: yisaer <[email protected]> fix test Signed-off-by: yisaer <[email protected]>

Yisaer · 2022-04-26T07:16:42Z

/run-all-tests

xuyifangreeneyes · 2022-04-27T12:28:02Z

statistics/handle/lru_cache.go

+	// We first push it into cache front here, if the element TotalColTrackingMemUsage is 0, it will be
+	// removed form cache list in l.maintainList
+	element = l.cache.PushFront(newCacheEntry)
+	l.maintainList(element, newCacheEntry, tblMemUsage.TotalColTrackingMemUsage(), 1)


It seems that newCacheEntry would not be used in maintainList? Is it better to pass nil in?

Emmm, I' not sure about it.

xuyifangreeneyes · 2022-04-27T13:03:43Z

statistics/handle/lru_cache.go

+		element.Value.(*cacheItem).value = value
+		element.Value.(*cacheItem).tblMemUsage = tblMemUsage
+		l.calculateCost(tblMemUsage, oldMemUsage)
+		l.maintainList(element, nil, tblMemUsage.TotalColTrackingMemUsage(), oldMemUsage.TotalColTrackingMemUsage())


Is it possible that oldMemUsage.TotalColTrackingMemUsage() == 0 and newMemUsage.TotalColTrackingMemUsage() > 0? If it is possible, we will add nil into list.

chrysan · 2022-04-28T11:21:20Z

metrics/stats.go

+		Subsystem: "statistics",
+		Name:      "cache_mem_usage",
+		Help:      "The Gauge of stats cache mem usage",
+	}, []string{LblType})


Are you going to make changes to tidb.json in another PR?

chrysan · 2022-04-28T13:08:15Z

statistics/handle/lru_cache.go

+	l.evictIfNeeded()
+	totalCost := l.totalCost
+	trackingCost := l.trackingCost
+	l.Unlock()


Better to start with defer unless your logic between Lock and Unlock is quite simple.

chrysan · 2022-04-28T13:20:00Z

statistics/handle/lru_cache.go

+	if !exists {
+		return nil, false
+	}
+	l.cache.MoveToFront(element)


Agree, the information about which columns are used after getTableStats is everywhere. We can leverage CollectColumnStatsUsage later to maintain LRU of columns.

chrysan · 2022-04-28T13:37:29Z

statistics/handle/lru_cache.go

+	if !exists {
+		return nil, false
+	}
+	l.cache.MoveToFront(element)


And should only MoveToFront for GetByQuery.

chrysan · 2022-04-28T13:59:17Z

statistics/handle/lru_cache.go

+
+func (l *internalLRUCache) put(key int64, value *statistics.Table, tblMemUsage *statistics.TableMemoryUsage, tryEvict bool) {
+	// If the item TotalColTrackingMemUsage is larger than capacity, we will drop some structures in order to put it in cache
+	for l.capacity < tblMemUsage.TotalColTrackingMemUsage() {


Why do we need this forloop?

chrysan · 2022-04-28T14:14:45Z

statistics/handle/lru_cache.go

+func (l *internalLRUCache) maintainList(element *list.Element, item *cacheItem, newTotalColTrackingMemUsage, oldTotalColTrackingMemUsage int64) *list.Element {
+	if oldTotalColTrackingMemUsage > 0 {
+		if newTotalColTrackingMemUsage > 0 {
+			l.cache.MoveToFront(element)


I think we don't need to MoveToFront for update. It could be the table stats is updated by auto-analyze, it means nothing to LRU.
And I think oldTotalColTrackingMemUsage=0 doesn't mean the element does not exist, it could be the CMSketch of this table are all evicted.
I think we don't need to maintainList with memUsage, just do PushFront for new inserted table, do Remove for deleted table, and do MoveToFront for table GetByQuery.

Just keep LRU list consistent with map, the only usage of LRU list is to provide the order for evict.

chrysan · 2022-04-28T14:30:35Z

statistics/handle/lru_cache.go

+			item.tblMemUsage = newMemUsage
+			l.calculateCost(newMemUsage, oldMemUsage)
+			if l.underCapacity() {
+				break


Since we are maintaining LRU at Table level, why not evict CMSketch for all columns of this table directly to make it simple.

chrysan · 2022-04-28T14:44:42Z

statistics/handle/lru_cache.go

+	l.calculateCost(tblMemUsage, &statistics.TableMemoryUsage{})
+	// We first push it into cache front here, if the element TotalColTrackingMemUsage is 0, it will be
+	// removed form cache list in l.maintainList
+	element = l.cache.PushFront(newCacheEntry)


Put means nothing to "Last recently used", it could be updates when reading latest stats from kv.

ti-chi-bot added 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 Apr 21, 2022

Yisaer requested review from xuyifangreeneyes and chrysan April 21, 2022 05:29

add lru statscache

734dfbf

Signed-off-by: yisaer <[email protected]> add lru statscache Signed-off-by: yisaer <[email protected]> add lru statscache Signed-off-by: yisaer <[email protected]>

Yisaer force-pushed the introduce_lru_statscache branch from a4203d7 to 734dfbf Compare April 21, 2022 07:29

Yisaer added 2 commits April 21, 2022 16:15

add lru statscache

0c31b3a

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

add lru statscache

d24980c

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

hawkingrei reviewed Apr 21, 2022

View reviewed changes

statistics/handle/lru_cache.go Show resolved Hide resolved

xuyifangreeneyes reviewed Apr 22, 2022

View reviewed changes

statistics/handle/lru_cache.go Outdated Show resolved Hide resolved

chrysan reviewed Apr 22, 2022

View reviewed changes

metrics/stats.go Outdated Show resolved Hide resolved

chrysan reviewed Apr 22, 2022

View reviewed changes

metrics/stats.go Outdated Show resolved Hide resolved

chrysan reviewed Apr 22, 2022

View reviewed changes

address the comment

48eb8ef

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

Yisaer requested review from chrysan, xuyifangreeneyes and hawkingrei April 24, 2022 04:38

xuyifangreeneyes reviewed Apr 25, 2022

View reviewed changes

statistics/handle/lru_cache.go Show resolved Hide resolved

statistics/handle/lru_cache.go Show resolved Hide resolved

statistics/handle/lru_cache_test.go Show resolved Hide resolved

Yisaer added 2 commits April 26, 2022 12:20

address the comment

e8cbac2

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

address the comment

1341eb0

Signed-off-by: yisaer <[email protected]> fix test Signed-off-by: yisaer <[email protected]> fix test Signed-off-by: yisaer <[email protected]>

Yisaer force-pushed the introduce_lru_statscache branch from dbfac49 to 1341eb0 Compare April 26, 2022 06:36

Yisaer requested a review from xuyifangreeneyes April 26, 2022 10:24

xuyifangreeneyes reviewed Apr 27, 2022

View reviewed changes

chrysan reviewed Apr 28, 2022

View reviewed changes

Yisaer closed this Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statistics: add lru cache implement for statsCacheInner #34145

statistics: add lru cache implement for statsCacheInner #34145

Yisaer commented Apr 21, 2022 •

edited

Loading

ti-chi-bot commented Apr 21, 2022

sre-bot commented Apr 21, 2022 •

edited

Loading

chrysan Apr 22, 2022

Yisaer Apr 24, 2022 •

edited

Loading

chrysan Apr 28, 2022

chrysan Apr 28, 2022

Yisaer commented Apr 26, 2022

xuyifangreeneyes Apr 27, 2022

xuyifangreeneyes Apr 27, 2022

xuyifangreeneyes Apr 27, 2022

chrysan Apr 28, 2022

chrysan Apr 28, 2022

chrysan Apr 28, 2022

chrysan Apr 28, 2022

chrysan Apr 28, 2022

chrysan Apr 28, 2022

chrysan Apr 28, 2022

chrysan Apr 28, 2022

chrysan Apr 28, 2022

statistics: add lru cache implement for statsCacheInner #34145

statistics: add lru cache implement for statsCacheInner #34145

Conversation

Yisaer commented Apr 21, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 21, 2022

sre-bot commented Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Yisaer Apr 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yisaer commented Apr 26, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yisaer commented Apr 21, 2022 •

edited

Loading

sre-bot commented Apr 21, 2022 •

edited

Loading

Yisaer Apr 24, 2022 •

edited

Loading