Skip to content
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

KVStore: Record table-wise memory usage and warn when it goes beyond the threshold #9835

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

CalvinNeo
Copy link
Member

@CalvinNeo CalvinNeo commented Jan 27, 2025

What problem does this PR solve?

Issue Number: ref #9722

Problem Summary:

  • Introduce a struct RegionTableSize which is shared by a Table and all its related Regions. Once registered, a region will update its memory change to this struct, so the table would learn.
  • Introduce maybeWarnMemoryLimitByTable and resetWarnMemoryLimitByTable, to log when the memory of a table goes beyond threshold.

Both of the systems works under the path that:

  • When a region is created, either for prehandling or inserting, it would register it to RegionTable, and get its shared RegionTableSize.
  • Ever since, the insert/delete from the Region would also reflect on this shared RegionTableSize.
  • After an insertion, if the memory consumed by a table associated with the given region is reported to go beyond the threshold propotion, and the total memory used is also beyond the given memory limit(which is a very corner case), then we will issue a log.
  • Afterwards, if the memory usage somehow goes upper, we don't print the log again for the same table.
  • If the memory goes down by committing txns, or applying snapshots, we will reset the warning. So if the table still consumes much memory, a new log will be printed.
  • If the region is splitted or merged, the table's memory usage is not affected. If the region is destroyed, the table's memory would be decresed.

Also:

  • Remove some useless structures in RegionTable.
  • Make safe ts related codes into SafeTsMgr.
  • Rename some methods in Region to reflect what they actually do.

What is changed and how it works?

Record table-wise memory usage and warn when it goes beyond threshold

Check List

Tests

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

Side effects

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

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. labels Jan 27, 2025
Copy link
Contributor

ti-chi-bot bot commented Jan 27, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from calvinneo, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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 ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 27, 2025
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
@ti-chi-bot 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 Feb 5, 2025
CalvinNeo and others added 5 commits February 5, 2025 17:55
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
@CalvinNeo CalvinNeo changed the title dnm Record table-wise memory usage Record table-wise memory usage Feb 7, 2025
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
@ti-chi-bot ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 7, 2025
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
CalvinNeo and others added 4 commits February 12, 2025 13:15
Co-authored-by: JaySon <tshent@qq.com>
Co-authored-by: JaySon <tshent@qq.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Comment on lines 63 to 67
RegionTable::InternalRegion & RegionTable::insertRegion(Table & table, const Region & region)
{
const auto range = region.getRange();
return insertRegion(table, *range, region.id());
return insertRegion(table, *range, region);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make line 108 in RegionTable::getOrInsertRegion as return insertRegion(table, *region.getRange(), region); and remove this redundant function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

Comment on lines +55 to +58

Type total() const { return payload + decoded; }

size_t totalSize() const { return payload + decoded; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need two method for the same result? Maybe just adding explicit cast when want to assign the result to differnet type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

total() needs no casting, but is not used very much,
totalSize() needs casting, but is widely used in test, so it is we just write it here to eliminate some static_casting

std::atomic_int64_t table_size;
std::atomic_bool warned;
};
using RegionTableCtx = std::shared_ptr<RegionTableCtxInner>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename to RegionTableCtxPtr? Most alias of shared_ptr are xxxPtr in TiFlash.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just keep those function name to make them shorter?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just keep those function name to make them shorter?

I think its OK.

CalvinNeo and others added 8 commits February 12, 2025 14:57
Co-authored-by: jinhelin <linjinhe33@gmail.com>
Co-authored-by: JaySon <tshent@qq.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
f
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: Calvin Neo <calvinneo1995@gmail.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Comment on lines +411 to +413
// If there are data flow in, we will check if the memory is exhaused.
auto limit = tmt.getKVStore()->getKVStoreMemoryLimit();
auto current = real_rss.load();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • If KVStoreMemoryLimit (max_memory_usage_for_all_queries) is explicitly set to 0, now it will not print any logging. IMO, for this case we should use the server memory capacity * 80% as the warning limit.
  • Only print log when the region_table_size > 50% * current is too large. I think we may easily miss the logging for track. I recommend to lower the table level memory threshold to min(20% * current, 10GiB) to increase the recall rate for logging down info.

Copy link
Member Author

@CalvinNeo CalvinNeo Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • OK
  • We'd better substract "start_time" memory which could be significant if we set the memory limit to kind of a small value like 4GiB.

Signed-off-by: JaySon-Huang <tshent@qq.com>
@CalvinNeo
Copy link
Member Author

/hold I want to re-perform a test over the new pr.

@ti-chi-bot ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2025
@CalvinNeo
Copy link
Member Author

Test
image
image

[2025/02/14 16:01:37.575 +08:00] [INFO] [Region.cpp:438] ["Memory limit exceeded, current=2756718592 limit=4294967297 table_id=164 keyspace_id=4294967295 region_id=3406 from=put"] [thread_id=181]

When rss is about 3.63 GiB, the KVStore memory consumption is about 619MB, which is about 16.7% of the rss.

The reported point is at 2629MB.

@CalvinNeo
Copy link
Member Author

CalvinNeo commented Feb 14, 2025

Another test that prints the table size

[2025/02/14 16:37:01.643 +08:00] [INFO] [Region.cpp:439] ["Memory limit exceeded, current=2866593792 limit=4294967297 table_size=143347752 table_id=168 keyspace_id=4294967295 region_id=3458 from=put"] [thread_id=353]

So the table size is 143347752 bytes(137MiB), while the current is 2866593792 bytes(2.66G), about 5%.

image

image

It shows the KVStore size is 630MB, while the usage is 3.74GiB, about 16%.

Consider the base memory usage is 2.37 GiB. It shows a byte cost in KVStore will result in 2 bytes cost in memory usage.

  • 137MiB(by log) vs 0.29GiB 46.9%
  • 630MB(by grafana) vs 1.37GiB 44.9%

Suppose the memory starts from X, and grows to Y, which is our "warning limit". And if it is all due to one table in KVStore, the table's "table_size" should be (Y - X) / 2.

@CalvinNeo
Copy link
Member Author

CalvinNeo commented Feb 14, 2025

New method

[2025/02/14 18:59:53.199 +08:00] [INFO] [Server.cpp:975] ["Global memory status: computed_limit 4294967297 tranquil_time_rss 737959936, cur_virt_size 2888970240, capacity 403755778048"] [thread_id=1]

[2025/02/14 19:09:16.426 +08:00] [INFO] [Region.cpp:444] ["Memory limit exceeded, current=3889836032 limit=4294967297 limit_size=945562828.8 table_size=641429928 table_id=183 keyspace_id=4294967295 region_id=4226 from=put"] [thread_id=368]

image

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants