statistics: add lru cache implement for statsCacheInner #34323
Conversation
Signed-off-by: yisaer <[email protected]>
Signed-off-by: yisaer <[email protected]>
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
release-note-none
|
Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/ce540c4c2b29de80d5f3a53e07a01ab72c05bdaf |
Signed-off-by: yisaer <[email protected]>
ti-chi-bot
added
the
do-not-merge/work-in-progress
ti-chi-bot
removed
the
do-not-merge/work-in-progress
Signed-off-by: yisaer <[email protected]>
Signed-off-by: yisaer <[email protected]>
Signed-off-by: yisaer <[email protected]> address the comment Signed-off-by: yisaer <[email protected]>
Yisaer
force-pushed
the
introduce_statscache_lru
branch
from
33abdad to
107d4c0
Compare
Signed-off-by: yisaer <[email protected]>
Signed-off-by: yisaer <[email protected]>
statistics/handle/lru_cache.go
Outdated
| s.lru.put(tblID, idxID, idx, newTblMem.IndicesMemUsage[idxID], true, false) | ||
| } | ||
| // tbl mem usage might be changed due to evict | ||
| element.tblMemUsage = element.tbl.MemoryUsage() |
There was a problem hiding this comment.
Is it duplicated with element.tbl.MemoryUsage() which is called in onEvict?
There was a problem hiding this comment.
right, it's unnecessary now, updated.
Signed-off-by: yisaer <[email protected]>
Yisaer
force-pushed
the
introduce_statscache_lru
branch
from
55a7a68 to
e45dc60
Compare
|
|
||
| type lruCacheItem struct { | ||
| tblID int64 | ||
| id int64 |
There was a problem hiding this comment.
Do we just use Index.ID and Column.ID to set lruCacheItem.id here?
There was a problem hiding this comment.
We will differentiate between index and column in the next pr.
| defer s.RUnlock() | ||
| totalCost := int64(0) | ||
| for tblID, ele := range s.elements { | ||
| s.freshTableCost(tblID, ele) |
There was a problem hiding this comment.
Is it safe to call freshTableCost with RLock? freshTableCost call innerItemLruCache.put which does some writes.
Signed-off-by: yisaer <[email protected]>
| if !ok { | ||
| return nil, false | ||
| } | ||
| return ele.Value.(*lruCacheItem), true |
There was a problem hiding this comment.
Do we need to call c.cache.MoveToFront here?
Signed-off-by: yisaer <[email protected]>
Signed-off-by: yisaer <[email protected]>
| item := curr.Value.(*lruCacheItem) | ||
| newLRU.put(item.tblID, item.id, item.innerItem, item.innerMemUsage, false, false) | ||
| curr = curr.Prev() | ||
| } |
There was a problem hiding this comment.
Do we need to copy c.onEvict here?
There was a problem hiding this comment.
no, onEvict will be set in newStatsLruCache.
Signed-off-by: yisaer <[email protected]>
Signed-off-by: yisaer <[email protected]>
ti-chi-bot
added
status/LGT2
Signed-off-by: yisaer <[email protected]>
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 39edbe3
|
ti-chi-bot
added
the
status/can-merge
TiDB MergeCI notify
|
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
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.