[scudo] On Android, only release when changing M_DECAY_TIME to 0.

[cferris1000]

There is no reason to make a force release to OS call when enabling the decay timer. When disabling, it's logical since the intent is to start using less memory. When enabling, it's not logical because it usually happens early in the process life cycle and a caller can do it themselves if necessary.

This is in Android only code right now, so only affects Android devices.

[llvmbot]

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

Author: Christopher Ferris (cferris1000)

Changes

There is no reason to make a force release to OS call when enabling the decay timer. When disabling, it's logical since the intent is to start using less memory. When enabling, it's not logical because it usually happens early in the process life cycle and a caller can do it themselves if necessary.

This is in Android only code right now, so only affects Android devices.

---

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

1 Files Affected:

• (modified) compiler-rt/lib/scudo/standalone/wrappers_c.inc (+6-5)

diff --git a/compiler-rt/lib/scudo/standalone/wrappers_c.inc b/compiler-rt/lib/scudo/standalone/wrappers_c.inc
index 59f3fb0962f8b..d805ace54b0ca 100644
--- a/compiler-rt/lib/scudo/standalone/wrappers_c.inc
+++ b/compiler-rt/lib/scudo/standalone/wrappers_c.inc
@@ -247,16 +247,17 @@ 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);
-
       // The values allowed on Android are {-1, 0, 1}. "1" means the longest
       // interval.
       CHECK(value >= -1 && value <= 1);
       if (value == 1)
         value = INT32_MAX;
+      else if (value == 0) {
+        // Before disabling 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);
+      }
     }
 
     SCUDO_ALLOCATOR.setOption(scudo::Option::ReleaseInterval,

[cferris1000]

While adding tracing, I found one place where the release to OS call happens early in the process life and it doesn't make a lot of sense. That's why I'm removing it.

[ChiaHungDuan]

While adding tracing, I found one place where the release to OS call happens early in the process life and it doesn't make a lot of sense. That's why I'm removing it.

I think the reason for doing a force-purge is that when switching back from 0 to 1, we want to make sure the "heap" is back to the managed status so our page releasing heuristic won't be impacted. Switching to 0 means the user is aware of the impact of unreleased pages thus doing a purge before that seems not showing much value ?! Not sure why I did it for both cases (maybe for consistency?). Given this reason, what do you think if we only do it for switch to 1?

[cferris1000]

While adding tracing, I found one place where the release to OS call happens early in the process life and it doesn't make a lot of sense. That's why I'm removing it.

I think the reason for doing a force-purge is that when switching back from 0 to 1, we want to make sure the "heap" is back to the managed status so our page releasing heuristic won't be impacted. Switching to 0 means the user is aware of the impact of unreleased pages thus doing a purge before that seems not showing much value ?! Not sure why I did it for both cases (maybe for consistency?). Given this reason, what do you think if we only do it for switch to 1?

I've been thinking about this, and really you need to know the previous value to do this right. For example, going from -1 to anything should probably trigger a purge. Maybe switching to -1 shouldn't trigger a purge, maybe. And hardly anyone switches to 0. Therefore, I don't think this saves much time so it's probably just better to leave this alone. The chance of doing this wrong seems high and leaving memory hanging around is not worth trying to fix this.