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.

Sign up for GitHub

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

Jump to bottom

Tracking memory usage of PagePacket and RemotePage in RN fetching pages #7667

Merged
merged 13 commits into from
Jun 29, 2023
Merged

Tracking memory usage of PagePacket and RemotePage in RN fetching pages #7667

merged 13 commits into from
Jun 29, 2023

Conversation

JinheLin
Copy link
Contributor

@JinheLin JinheLin commented Jun 16, 2023 •
edited
Loading

What problem does this PR solve?

Issue Number: ref #7670 ref #7628

Problem Summary: Tracking memory usage of RN fetching pages.

What is changed and how it works?

  • Add sub_root_of_query_storage_task_mem_trackers. If a memory tracker of storage tasks is driven by query, it should inherit sub_root_of_query_storage_task_mem_trackers.
  • Add fetch_pages_mem_tracker for tracking memory usage of fetching pages. It inherit from sub_root_of_query_storage_task_mem_trackers.
  • Tracking memory usage of PagePacket and RemotePage in fetching pages.
  • Add message about memory usage of sub_root_of_query_storage_task_mem_trackers and fetch_pages_mem_tracker when memory usage exceeded.

image
image

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
    • Write a lot of data to TiFlash, make data in DeltaValueSpace large.
    • Remove local cache of RNs.
    • Set configuration profiles.default.max_memory_usage_for_all_queries to 0.2.
    • Run select count(*) from t.
    • TiFlash throws exception of memory limit.
  • 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

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Jun 16, 2023
@JinheLin JinheLin force-pushed the fetch-page-mem branch 2 times, most recently from cd1a65b to 1ab9fa9 Compare June 16, 2023 09:24
@ti-chi-bot ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 19, 2023
@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 19, 2023
@JinheLin JinheLin changed the title WIP: Track memory usage of fetching pages. Tracking memory usage of PagePacket and RemotePage in RN fetching pages Jun 19, 2023
@JinheLin JinheLin force-pushed the fetch-page-mem branch 2 times, most recently from a5fbf8d to 4db3c7d Compare June 19, 2023 10:22
@JinheLin JinheLin marked this pull request as ready for review June 19, 2023 10:23
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2023
@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 20, 2023
@JinheLin
Copy link
Contributor Author

/run-all-tests

@JinheLin JinheLin requested a review from bestwoody June 28, 2023 07:51
bestwoody
bestwoody reviewed Jun 28, 2023
View reviewed changes
dbms/src/Common/MemoryTracker.cpp Outdated
@@ -155,6 +171,11 @@ void MemoryTracker::alloc(Int64 size, bool check_memory_limit)
formatReadableSizeWithBinarySuffix(current_limit));
}

if (sub_root_of_query_storage_task_mem_trackers)
Copy link
Contributor

@bestwoody bestwoody Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

to achive a more detaied debug info, how about printing root_of_non_query , sub_root_of_query_storage_task_mem_trackers, fetch_pages_mem_tracker in every case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Format memory usage of these three MemoryTrackers in storageMemoryUsageDetail and add the message in every exception in MemoryTracker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exception message example:

Code: 0, e.displayText() = DB::TiFlashException: Memory limit exceeded caused by 'out of memory quota for data computing' : would use 756.00 B for data computing (attempt to allocate chunk of 256 bytes), limit of memory for data computing: 512.00 B. Memory usage of storage: non-query: peak=0.00 B, amount=0.00 B; query-storage-task: peak=0.00 B, amount=0.00 B; fetch-pages: peak=0.00 B, amount=0.00 B.

dbms/src/Common/MemoryTracker.cpp Outdated
@@ -65,6 +71,13 @@ static Poco::Logger * getLogger()
return logger;
}

static String memoryUsageDetail()
Copy link
Contributor

Choose a reason for hiding this comment

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

name is too generic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rename this function to storageMemoryUsageDetail.

@JinheLin
Copy link
Contributor Author

/run-unit-test

@JinheLin
Copy link
Contributor Author

/run-integration-test

@JinheLin JinheLin requested a review from bestwoody June 29, 2023 03:00
@JinheLin
Copy link
Contributor Author

/run-unit-test

@JinheLin
Copy link
Contributor Author

/run-integration-test

bestwoody
bestwoody approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@bestwoody bestwoody left a comment

Choose a reason for hiding this comment

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

lgtm

@ti-chi-bot ti-chi-bot bot added the lgtm label Jun 29, 2023
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jun 29, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bestwoody, JaySon-Huang

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:
  • OWNERS [JaySon-Huang,bestwoody]

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 removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 29, 2023
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jun 29, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-06-20 03:20:20.442940567 +0000 UTC m=+72985.844191013: ☑️ agreed by JaySon-Huang.
  • 2023-06-29 06:19:53.177396365 +0000 UTC m=+861358.578646814: ☑️ agreed by bestwoody.

@JinheLin
Copy link
Contributor Author

/merge

@JinheLin
Copy link
Contributor Author

/approved

@JinheLin
Copy link
Contributor Author

/run-integration-test

@JinheLin
Copy link
Contributor Author

/run-unit-test

@ti-chi-bot ti-chi-bot bot merged commit 7bd02a8 into pingcap:master Jun 29, 2023
JinheLin added a commit to JinheLin/tiflash-1 that referenced this pull request Jul 10, 2023
@JinheLin
Tracking memory usage of PagePacket and RemotePage in RN fetching…
67c0bb6 
… pages (pingcap#7667)

ref pingcap#7628, ref pingcap#7670
@JinheLin JinheLin deleted the fetch-page-mem branch June 4, 2024 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@windtalker windtalker windtalker left review comments

@bestwoody bestwoody bestwoody approved these changes

@JaySon-Huang JaySon-Huang JaySon-Huang approved these changes

@breezewish breezewish Awaiting requested review from breezewish

Assignees
No one assigned
Labels
approved lgtm 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JinheLin @windtalker @JaySon-Huang @bestwoody