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] Add utilization percentages for stats. #75101

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

cferris1000
Copy link
Contributor

Refactor the percentage display in the secondary code. Re-use that to display a utilization percentage when displaying fragmentation data.

Refactor the percentage display in the secondary code.
Re-use that to display a utilization percentage when
displaying fragmentation data.
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 11, 2023

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

Author: Christopher Ferris (cferris1000)

Changes

Refactor the percentage display in the secondary code. Re-use that to display a utilization percentage when displaying fragmentation data.


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

4 Files Affected:

  • (modified) compiler-rt/lib/scudo/standalone/common.h (+15)
  • (modified) compiler-rt/lib/scudo/standalone/primary32.h (+6-2)
  • (modified) compiler-rt/lib/scudo/standalone/primary64.h (+6-2)
  • (modified) compiler-rt/lib/scudo/standalone/secondary.h (+5-9)
diff --git a/compiler-rt/lib/scudo/standalone/common.h b/compiler-rt/lib/scudo/standalone/common.h
index 3581c946d1608..544a64ad3bf80 100644
--- a/compiler-rt/lib/scudo/standalone/common.h
+++ b/compiler-rt/lib/scudo/standalone/common.h
@@ -112,6 +112,21 @@ template <typename T> inline void shuffle(T *A, u32 N, u32 *RandState) {
   *RandState = State;
 }
 
+inline void computePercentage(uptr Numerator, uptr Denominator, uptr *Integral,
+                              uptr *Fractional) {
+  constexpr uptr Digits = 100;
+  if (Denominator == 0) {
+    *Integral = 0;
+    *Fractional = 0;
+    return;
+  }
+
+  *Integral = Numerator * Digits / Denominator;
+  *Fractional =
+      (((Numerator * Digits) % Denominator) * Digits + Denominator / 2) /
+      Denominator;
+}
+
 // Platform specific functions.
 
 extern uptr PageSizeCached;
diff --git a/compiler-rt/lib/scudo/standalone/primary32.h b/compiler-rt/lib/scudo/standalone/primary32.h
index 8281e02ba164c..df1caae94dadd 100644
--- a/compiler-rt/lib/scudo/standalone/primary32.h
+++ b/compiler-rt/lib/scudo/standalone/primary32.h
@@ -931,10 +931,14 @@ template <typename Config> class SizeClassAllocator32 {
         AllocatedPagesCount - Recorder.getReleasedPagesCount();
     const uptr InUseBytes = InUsePages * PageSize;
 
+    uptr Integral;
+    uptr Fractional;
+    computePercentage(BlockSize * InUseBlocks, InUsePages * PageSize, &Integral,
+                      &Fractional);
     Str->append("  %02zu (%6zu): inuse/total blocks: %6zu/%6zu inuse/total "
-                "pages: %6zu/%6zu inuse bytes: %6zuK\n",
+                "pages: %6zu/%6zu inuse bytes: %6zuK util: %2zu.%02zu%%\n",
                 ClassId, BlockSize, InUseBlocks, TotalBlocks, InUsePages,
-                AllocatedPagesCount, InUseBytes >> 10);
+                AllocatedPagesCount, InUseBytes >> 10, Integral, Fractional);
   }
 
   NOINLINE uptr releaseToOSMaybe(SizeClassInfo *Sci, uptr ClassId,
diff --git a/compiler-rt/lib/scudo/standalone/primary64.h b/compiler-rt/lib/scudo/standalone/primary64.h
index d1929ff7212f4..d1f175cd15a11 100644
--- a/compiler-rt/lib/scudo/standalone/primary64.h
+++ b/compiler-rt/lib/scudo/standalone/primary64.h
@@ -1130,10 +1130,14 @@ template <typename Config> class SizeClassAllocator64 {
         AllocatedPagesCount - Recorder.getReleasedPagesCount();
     const uptr InUseBytes = InUsePages * PageSize;
 
+    uptr Integral;
+    uptr Fractional;
+    computePercentage(BlockSize * InUseBlocks, InUsePages * PageSize, &Integral,
+                      &Fractional);
     Str->append("  %02zu (%6zu): inuse/total blocks: %6zu/%6zu inuse/total "
-                "pages: %6zu/%6zu inuse bytes: %6zuK\n",
+                "pages: %6zu/%6zu inuse bytes: %6zuK util: %2zu.%02zu%%\n",
                 ClassId, BlockSize, InUseBlocks, TotalBlocks, InUsePages,
-                AllocatedPagesCount, InUseBytes >> 10);
+                AllocatedPagesCount, InUseBytes >> 10, Integral, Fractional);
   }
 
   NOINLINE uptr releaseToOSMaybe(RegionInfo *Region, uptr ClassId,
diff --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index 8dc4c87fa7c6e..f52a4188bcf3a 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -155,20 +155,16 @@ template <typename Config> class MapAllocatorCache {
 
   void getStats(ScopedString *Str) {
     ScopedLock L(Mutex);
-    u32 Integral = 0;
-    u32 Fractional = 0;
-    if (CallsToRetrieve != 0) {
-      Integral = SuccessfulRetrieves * 100 / CallsToRetrieve;
-      Fractional = (((SuccessfulRetrieves * 100) % CallsToRetrieve) * 100 +
-                    CallsToRetrieve / 2) /
-                   CallsToRetrieve;
-    }
+    uptr Integral;
+    uptr Fractional;
+    computePercentage(SuccessfulRetrieves, CallsToRetrieve, &Integral,
+                      &Fractional);
     Str->append("Stats: MapAllocatorCache: EntriesCount: %d, "
                 "MaxEntriesCount: %u, MaxEntrySize: %zu\n",
                 EntriesCount, atomic_load_relaxed(&MaxEntriesCount),
                 atomic_load_relaxed(&MaxEntrySize));
     Str->append("Stats: CacheRetrievalStats: SuccessRate: %u/%u "
-                "(%u.%02u%%)\n",
+                "(%zu.%02zu%%)\n",
                 SuccessfulRetrieves, CallsToRetrieve, Integral, Fractional);
     for (CachedBlock Entry : Entries) {
       if (!Entry.isValid())

inline void computePercentage(uptr Numerator, uptr Denominator, uptr *Integral,
uptr *Fractional) {
constexpr uptr Digits = 100;
if (Denominator == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we want to assert a non-zero denominator. It seems to give the impression that 0/1 and 0/0 are the same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another case I saw in real life, and I was wondering if I should say 100% when the denominator is 0. Since 0% feels like the wrong value, 100% also seems kind of wrong, but more correct than 0%. My other thought was doing NA, but that seems like it would be very difficult to implement without changing a lot of things.

I would think setting this to 100% seems the least bad and the most correct, but what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, I think 100% is better! (Given that NA is more complicated)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set to 100, and modified the two locations that use it to reserve enough space for that 100%.

@@ -112,6 +112,21 @@ template <typename T> inline void shuffle(T *A, u32 N, u32 *RandState) {
*RandState = State;
}

inline void computePercentage(uptr Numerator, uptr Denominator, uptr *Integral,
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking if we want a Pair in Scudo but I didn't get a good use case to add it. This form works for me too. Will leave it to you to make the decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was hoping I could use std::pair, but I think that would be a bad idea. And adding our own definition feels like a lot of work for a single case that's not necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree with this

Copy link
Contributor

@ChiaHungDuan ChiaHungDuan left a comment

Choose a reason for hiding this comment

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

LGTM

@cferris1000 cferris1000 merged commit a8ef9c0 into llvm:main Dec 12, 2023
4 checks passed
@cferris1000 cferris1000 deleted the scudo_util branch December 12, 2023 00:42
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