[native] Init system used memory cache at the beginning of PeriodicMemoryChecker callback#26152
Closed
kletkavrubashku wants to merge 1 commit intoprestodb:masterfrom
Closed
[native] Init system used memory cache at the beginning of PeriodicMemoryChecker callback#26152kletkavrubashku wants to merge 1 commit intoprestodb:masterfrom
kletkavrubashku wants to merge 1 commit intoprestodb:masterfrom
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR seeds the system memory usage cache at the start of each PeriodicMemoryChecker callback and switches subsequent memory checks to use the cached value, also updating the test fixture to initialize the cache to maintain consistency. Class diagram for updated PeriodicMemoryChecker memory access methodsclassDiagram
class PeriodicMemoryChecker {
- cachedSystemUsedMemoryBytes_: int64_t
+ systemUsedMemoryBytes(): int64_t
+ cachedSystemUsedMemoryBytes() const: int64_t
+ start()
+ periodicCb()
+ maybeDumpHeap()
+ pushbackMemory()
}
PeriodicMemoryChecker : systemUsedMemoryBytes() updates cachedSystemUsedMemoryBytes_
PeriodicMemoryChecker : cachedSystemUsedMemoryBytes() returns cachedSystemUsedMemoryBytes_
PeriodicMemoryChecker : start() seeds cache before callback
PeriodicMemoryChecker : pushbackMemory() now uses cached value
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Collaborator
|
@kletkavrubashku has exported this pull request. If you are a Meta employee, you can view the originating diff in D83218739. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider renaming systemUsedMemoryBytes() to reflect its side effect (e.g., refreshSystemMemoryCache()) and providing a pure accessor for reading the cached value.
- Rather than seeding cachedSystemUsedMemoryBytes_ in the test subclass, consider adding a protected initializer or setter in PeriodicMemoryChecker to simplify test setup and encapsulate implementation details.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider renaming systemUsedMemoryBytes() to reflect its side effect (e.g., refreshSystemMemoryCache()) and providing a pure accessor for reading the cached value.
- Rather than seeding cachedSystemUsedMemoryBytes_ in the test subclass, consider adding a protected initializer or setter in PeriodicMemoryChecker to simplify test setup and encapsulate implementation details.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
kletkavrubashku
changed the title
…riodicMemoryChecker callback (prestodb#26152) Summary: in Currently `PeriodicMemoryChecker` and descendants use these 2 methods to get current system used memory: - `systemUsedMemoryBytes`: ``` int64_t systemUsedMemoryBytes() { const auto currentMemBytes = ...; cachedSystemUsedMemoryBytes_ = currentMemBytes; return currentMemBytes; } ``` - `cachedSystemUsedMemoryBytes` ``` int64_t cachedSystemUsedMemoryBytes() const { return cachedSystemUsedMemoryBytes_; } ``` Both methods are `protected` and don't become public in descendants, so their usage is limited by the classes itself. We get value from cache only if there is a prior `systemUsedMemoryBytes` call, before that the cache is empty. All `systemUsedMemoryBytes` and `cachedSystemUsedMemoryBytes` calls only happen inside the callback, this makes it easy to maintain the invariants. ### The problem In one of the upcoming diffs I'll need an access to the value of current system used memory inside `periodicCb()` implementation. The straightforward approach would be to just call `systemUsedMemoryBytes`, but it's a syscall and I noticed, that we can do some optimization in that area. We can do `systemUsedMemoryBytes()` call as the very first line in the callback and then everywhere below use cached value or actual value depends on needs. As example in `PeriodicMemoryChecker::pushbackMemory` we can also get the value from cache, and then update it by calling `systemUsedMemoryBytes()` after the pushback, which we're already doing in the code. Differential Revision: D83218739
kletkavrubashku
force-pushed
the
export-D83218739
branch
from
ac47c1c to
358fa61
Compare
Contributor
Author
|
Merged: #26347 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
in Currently
PeriodicMemoryCheckerand descendants use these 2 methods to get current system used memory:systemUsedMemoryBytes:cachedSystemUsedMemoryBytesBoth methods are
protectedand don't become public in descendants, so their usage is limited by the classes itself. We get value from cache only if there is a priorsystemUsedMemoryBytescall, before that the cache is empty. AllsystemUsedMemoryBytesandcachedSystemUsedMemoryBytescalls only happen inside the callback, this makes it easy to maintain the invariants.The problem
In one of the upcoming diffs I'll need an access to the value of current system used memory inside
periodicCb()implementation. The straightforward approach would be to just callsystemUsedMemoryBytes, but it's a syscall and I noticed, that we can do some optimization in that area. We can dosystemUsedMemoryBytes()call as the very first line in the callback and then everywhere below use cached value or actual value depends on needs. As example inPeriodicMemoryChecker::pushbackMemorywe can also get the value from cache, and then update it by callingsystemUsedMemoryBytes()after the pushback, which we're already doing in the code.Differential Revision: D83218739
== NO RELEASE NOTE ==