[scudo] Move the decision making of page release to hasChanceToReleasePages()

[Chia-hungDuan]

Summary

This change is only in SizeClassAllocator32. SizeClassAllocator64 has
it implemented.

[cferris]

One small suggested change.

compiler-rt/lib/scudo/standalone/primary32.h

1023

I think this check can be removed because it's checked before the call. Unless you plan to call this from other places where ForceAll could be set.

[Chia-hungDuan]

Remove redundant check

compiler-rt/lib/scudo/standalone/primary32.h

1023

Nice Catch!

[cferris]

LGTM.

compiler-rt/lib/scudo/standalone/primary32.h

NOINLINE uptr releaseToOSMaybe(SizeClassInfo *Sci, uptr ClassId,
     const uptr BytesInFreeList =
         Sci->AllocatedUser -
         (Sci->FreeListInfo.PoppedBlocks - Sci->FreeListInfo.PushedBlocks) *
             BlockSize;
 
     if (UNLIKELY(BytesInFreeList == 0))
       return 0;
 
-    if (BytesInFreeList <= Sci->ReleaseInfo.BytesInFreeListAtLastCheckpoint)
-      Sci->ReleaseInfo.BytesInFreeListAtLastCheckpoint = BytesInFreeList;
-
-    // Always update `BytesInFreeListAtLastCheckpoint` with the smallest value
-    // so that we won't underestimate the releasable pages. For example, the
-    // following is the region usage,
-    //
-    //  BytesInFreeListAtLastCheckpoint   AllocatedUser
-    //                v                         v
-    //  |--------------------------------------->
-    //         ^                   ^
-    //  BytesInFreeList     ReleaseThreshold
-    //
-    // In general, if we have collected enough bytes and the amount of free
-    // bytes meets the ReleaseThreshold, we will try to do page release. If we
-    // don't update `BytesInFreeListAtLastCheckpoint` when the current
-    // `BytesInFreeList` is smaller, we may take longer time to wait for enough
-    // freed blocks because we miss the bytes between
-    // (BytesInFreeListAtLastCheckpoint - BytesInFreeList).
-    const uptr PushedBytesDelta =
-        BytesInFreeList - Sci->ReleaseInfo.BytesInFreeListAtLastCheckpoint;
-    if (PushedBytesDelta < PageSize && ReleaseType != ReleaseToOS::ForceAll)
-      return 0;
-
-    const bool CheckDensity =
-        isSmallBlock(BlockSize) && ReleaseType != ReleaseToOS::ForceAll;
-    // Releasing smaller blocks is expensive, so we want to make sure that a
-    // significant amount of bytes are free, and that there has been a good
-    // amount of batches pushed to the freelist before attempting to release.
-    if (CheckDensity && ReleaseType == ReleaseToOS::Normal)
-      if (PushedBytesDelta < Sci->AllocatedUser / 16U)
-        return 0;
-
-    if (ReleaseType == ReleaseToOS::Normal) {
-      const s32 IntervalMs = atomic_load_relaxed(&ReleaseToOsIntervalMs);
-      if (IntervalMs < 0)
-        return 0;
-
-      // The constant 8 here is selected from profiling some apps and the number
-      // of unreleased pages in the large size classes is around 16 pages or
-      // more. Choose half of it as a heuristic and which also avoids page
-      // release every time for every pushBlocks() attempt by large blocks.
-      const bool ByPassReleaseInterval =
-          isLargeBlock(BlockSize) && PushedBytesDelta > 8 * PageSize;
-      if (!ByPassReleaseInterval) {
-        if (Sci->ReleaseInfo.LastReleaseAtNs +
-                static_cast<u64>(IntervalMs) * 1000000 >
-            getMonotonicTimeFast()) {
-          // Memory was returned recently.
-          return 0;
-        }
-      }
-    } // if (ReleaseType == ReleaseToOS::Normal)
+    // ====================================================================== //
+    // Check if we have enough free blocks and if it's worth doing a page
+    // release.
+    // ====================================================================== //
+    if (ReleaseType != ReleaseToOS::ForceAll &&
+        !hasChanceToReleasePages(Sci, BlockSize, BytesInFreeList,
+                                 ReleaseType)) {
 
     const uptr First = Sci->MinRegionIndex;
     const uptr Last = Sci->MaxRegionIndex;
     DCHECK_NE(Last, 0U);
     DCHECK_LE(First, Last);
     uptr TotalReleasedBytes = 0;
     const uptr Base = First * RegionSize;
     const uptr NumberOfRegions = Last - First + 1U;
for (BatchGroup &BG : Sci->FreeListInfo.BlockList) {
         continue;
 
       // TransferBatches are pushed in front of BG.Batches. The first one may
       // not have all caches used.
       const uptr NumBlocks = (BG.Batches.size() - 1) * BG.MaxCachedPerBatch +
                              BG.Batches.front()->getCount();
       const uptr BytesInBG = NumBlocks * BlockSize;
 
-      if (ReleaseType != ReleaseToOS::ForceAll &&
-          BytesInBG <= BG.BytesInBGAtLastCheckpoint) {
+      if (ReleaseType != ReleaseToOS::ForceAll) {
+        if (BytesInBG <= BG.BytesInBGAtLastCheckpoint) {
           BG.BytesInBGAtLastCheckpoint = BytesInBG;
           continue;
         }
+
         const uptr PushedBytesDelta = BytesInBG - BG.BytesInBGAtLastCheckpoint;
-      if (ReleaseType != ReleaseToOS::ForceAll && PushedBytesDelta < PageSize)
+        if (PushedBytesDelta < PageSize)
           continue;
 
-      // Given the randomness property, we try to release the pages only if the
-      // bytes used by free blocks exceed certain proportion of allocated
+        // Given the randomness property, we try to release the pages only if
+        // the bytes used by free blocks exceed certain proportion of allocated
         // spaces.
-      if (CheckDensity && (BytesInBG * 100U) / AllocatedGroupSize <
+        if (isSmallBlock(BlockSize) && (BytesInBG * 100U) / AllocatedGroupSize <
                                            (100U - 1U - BlockSize / 16U)) {
           continue;
         }
+      }
 
       // TODO: Consider updating this after page release if `ReleaseRecorder`
       // can tell the releasd bytes in each group.
       BG.BytesInBGAtLastCheckpoint = BytesInBG;
 
       const uptr MaxContainedBlocks = AllocatedGroupSize / BlockSize;
       const uptr RegionIndex = (GroupBase - Base) / RegionSize;
 
if (Recorder.getReleasedRangesCount() > 0) {
       Sci->ReleaseInfo.LastReleasedBytes = Recorder.getReleasedBytes();
       TotalReleasedBytes += Sci->ReleaseInfo.LastReleasedBytes;
     }
     Sci->ReleaseInfo.LastReleaseAtNs = getMonotonicTimeFast();
 
     return TotalReleasedBytes;
   }
 
+  bool hasChanceToReleasePages(SizeClassInfo *Sci, uptr BlockSize,
+                               uptr BytesInFreeList, ReleaseToOS ReleaseType)
+      REQUIRES(Sci->Mutex) {
+    DCHECK_GE(Sci->FreeListInfo.PoppedBlocks, Sci->FreeListInfo.PushedBlocks);
+    const uptr PageSize = getPageSizeCached();
+
+    if (BytesInFreeList <= Sci->ReleaseInfo.BytesInFreeListAtLastCheckpoint)
+      Sci->ReleaseInfo.BytesInFreeListAtLastCheckpoint = BytesInFreeList;
+
+    // Always update `BytesInFreeListAtLastCheckpoint` with the smallest value
+    // so that we won't underestimate the releasable pages. For example, the
+    // following is the region usage,
+    //
+    //  BytesInFreeListAtLastCheckpoint   AllocatedUser
+    //                v                         v
+    //  |--------------------------------------->
+    //         ^                   ^
+    //  BytesInFreeList     ReleaseThreshold
+    //
+    // In general, if we have collected enough bytes and the amount of free
+    // bytes meets the ReleaseThreshold, we will try to do page release. If we
+    // don't update `BytesInFreeListAtLastCheckpoint` when the current
+    // `BytesInFreeList` is smaller, we may take longer time to wait for enough
+    // freed blocks because we miss the bytes between
+    // (BytesInFreeListAtLastCheckpoint - BytesInFreeList).
+    const uptr PushedBytesDelta =
+        BytesInFreeList - Sci->ReleaseInfo.BytesInFreeListAtLastCheckpoint;
+    if (PushedBytesDelta < PageSize)
+      return false;
+
+    // Releasing smaller blocks is expensive, so we want to make sure that a
+    // significant amount of bytes are free, and that there has been a good
+    // amount of batches pushed to the freelist before attempting to release.
+    if (isSmallBlock(BlockSize) && ReleaseType == ReleaseToOS::Normal)
+      if (PushedBytesDelta < Sci->AllocatedUser / 16U)
+        return false;
+
+    if (ReleaseType == ReleaseToOS::Normal) {
+      const s32 IntervalMs = atomic_load_relaxed(&ReleaseToOsIntervalMs);
+      if (IntervalMs < 0)
+        return false;
+
+      // The constant 8 here is selected from profiling some apps and the number
+      // of unreleased pages in the large size classes is around 16 pages or
+      // more. Choose half of it as a heuristic and which also avoids page
+      // release every time for every pushBlocks() attempt by large blocks.
+      const bool ByPassReleaseInterval =
+          isLargeBlock(BlockSize) && PushedBytesDelta > 8 * PageSize;
+      if (!ByPassReleaseInterval) {
+        if (Sci->ReleaseInfo.LastReleaseAtNs +
+                static_cast<u64>(IntervalMs) * 1000000 >
+            getMonotonicTimeFast()) {
+          // Memory was returned recently.
+          return false;
+        }
+      }
+    } // if (ReleaseType == ReleaseToOS::Normal)
+
+    return true;
+  }
+
   SizeClassInfo SizeClassInfoArray[NumClasses] = {};
 
   HybridMutex ByteMapMutex;
   // Track the regions in use, 0 is unused, otherwise store ClassId + 1.
   ByteMap PossibleRegions GUARDED_BY(ByteMapMutex) = {};
   atomic_s32 ReleaseToOsIntervalMs = {};
   // Unless several threads request regions simultaneously from different size
   // classes, the stash rarely contains more than 1 entry.