-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: some tiny optimizations to reduce infoschema v2 memory #53242
Conversation
Hi @tiancaiamao. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #53242 +/- ##
================================================
+ Coverage 72.4582% 74.5301% +2.0719%
================================================
Files 1493 1493
Lines 429362 429696 +334
================================================
+ Hits 311108 320253 +9145
+ Misses 98985 89537 -9448
- Partials 19269 19906 +637
Flags with carried forward coverage won't be shown. Click here to find out more.
|
/retest-required |
@tiancaiamao: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hawkingrei, ywqzzy 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:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
/retest |
What problem does this PR solve?
Issue Number: ref #50959
Problem Summary:
What changed and how does it work?
SchemaTables()
toSchemaTableInfos()
SchemaTables()
is heavier thanSchemaTableInfos()
because the former need to load model.TableInfo from meta kv first and then buildtable.Table
frommodel.TableInfo
The caller only need the
model.TableInfo
, so change them toSchemaTableInfos
.I found some leaky memory occupation in infoschema v2:
It turn out to be that the
model.DBInfo
reference the Tables, so the memoy of json data can not be freed.SchemaTables()
Fill cache in the list API cause the memory thrash and also CPU thrash.
Especally I find the cost of
Sizeof()
is much higher than estimated.When we add to cache or evict cache, we need to call
Sizeof()
to calculate the size of an object.Skip refill cache in
SchemaTables()
can avoid much of that.Check List
Tests
I create a lot of partition tables and watch the memory usage.
The first issue is co-exist of infoschema v1 and v2, cause the memory usage doubled.
We can workaround it by setting
DefTiDBSchemaCacheSize > 0
rather than setting@@global.tidb_schema_cache_size
. (not done in this pull request)The second to third red square in the picture above shows the effect of this pull request.
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.