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

[scudo] Do a M_PURGE call before changing release interval on Android #87110

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

ChiaHungDuan
Copy link
Contributor

No description provided.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 29, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (ChiaHungDuan)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/87110.diff

1 Files Affected:

  • (modified) compiler-rt/lib/scudo/standalone/wrappers_c.inc (+5)
diff --git a/compiler-rt/lib/scudo/standalone/wrappers_c.inc b/compiler-rt/lib/scudo/standalone/wrappers_c.inc
index 56d8ef20156e28..21d5b7add51275 100644
--- a/compiler-rt/lib/scudo/standalone/wrappers_c.inc
+++ b/compiler-rt/lib/scudo/standalone/wrappers_c.inc
@@ -247,6 +247,11 @@ void SCUDO_PREFIX(malloc_postinit)() {
 INTERFACE WEAK int SCUDO_PREFIX(mallopt)(int param, int value) {
   if (param == M_DECAY_TIME) {
     if (SCUDO_ANDROID) {
+      // Before changing the interval, reset the memory usage status by doing a
+      // M_PURGE call so that we can minimize the impact of any unreleased pages
+      // introduced by interval transition.
+      SCUDO_ALLOCATOR.releaseToOS(scudo::ReleaseToOS::Force);
+
       if (value == 0) {
         // Will set the release values to their minimum values.
         value = INT32_MIN;

// Before changing the interval, reset the memory usage status by doing a
// M_PURGE call so that we can minimize the impact of any unreleased pages
// introduced by interval transition.
SCUDO_ALLOCATOR.releaseToOS(scudo::ReleaseToOS::Force);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be ForceAll? I presume no, but I wanted to ask.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the use case, They show similar memory footprint reduction but have noticeable latency difference. As a result, I think Force is good enough so far.

Copy link
Contributor

@cferris1000 cferris1000 left a comment

Choose a reason for hiding this comment

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

LGTM.

@ChiaHungDuan ChiaHungDuan merged commit 92d0d6f into llvm:main Apr 1, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants