[clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)

Teresa Johnson via cfe-commits cfe-commits at lists.llvm.org
Mon Jun 24 16:34:19 PDT 2024


================
@@ -216,6 +228,35 @@ u64 GetShadowCount(uptr p, u32 size) {
   return count;
 }
 
+// Accumulates the access count from the shadow for the given pointer and size.
+// See memprof_mapping.h for an overview on histogram counters.
+u64 GetShadowCountHistogram(uptr p, u32 size) {
+  u8 *shadow = (u8 *)HISTOGRAM_MEM_TO_SHADOW(p);
+  u8 *shadow_end = (u8 *)HISTOGRAM_MEM_TO_SHADOW(p + size);
+  u64 count = 0;
+  for (; shadow <= shadow_end; shadow++)
+    count += *shadow;
+  return count;
+}
+
+// If we use the normal approach from clearCountersWithoutHistogram, the
+// histogram will clear too much data and may overwrite shadow counters that are
+// in use. Likely because of rounding up the shadow_end pointer.
+// See memprof_mapping.h for an overview on histogram counters.
+void clearCountersHistogram(uptr addr, uptr size) {
+  u8 *shadow_8 = (u8 *)HISTOGRAM_MEM_TO_SHADOW(addr);
+  u8 *shadow_end_8 = (u8 *)HISTOGRAM_MEM_TO_SHADOW(addr + size);
+  for (; shadow_8 < shadow_end_8; shadow_8++) {
+    *shadow_8 = 0;
+  }
+}
+
+void clearCountersWithoutHistogram(uptr addr, uptr size) {
+  uptr shadow_beg = MEM_TO_SHADOW(addr);
+  uptr shadow_end = MEM_TO_SHADOW(addr + size - SHADOW_GRANULARITY) + 1;
+  REAL(memset)((void *)shadow_beg, 0, shadow_end - shadow_beg);
+}
+
 // Clears the shadow counters (when memory is allocated).
 void ClearShadow(uptr addr, uptr size) {
----------------
teresajohnson wrote:

I guess it works, because the shadow granularities are less than the page size, and because they are both scaling by the same 8 to 1 scale, and also I guess because the clear_shadow_mmap_threshold is an optimization and doesn't need to be exact. However, it still feels a little wonky to me (and also means that we have to do extra mapping operations here and again in `clearCounters*`). I do think I would prefer to have it be something like:

```
  uptr shadow_beg;
  uptr shadow_end;
    if (__memprof_histogram) {
      shadow_beg = HISTOGRAM_MEM_TO_SHADOW(addr);
      shadow_end = HISTOGRAM_MEM_TO_SHADOW(addr + size);
    } else {
      shadow_beg = MEM_TO_SHADOW(addr);
      shadow_end = MEM_TO_SHADOW(addr + size - SHADOW_GRANULARITY) + 1;
    }
    if (shadow_end - shadow_beg < common_flags()->clear_shadow_mmap_threshold) {
      REAL(memset)((void *)shadow_beg, 0, shadow_end - shadow_beg);
    } else {
...
```

I.e. set shadow_beg/end based on whether we are doing the histogramming or not, leave the rest as-is. Would that work?

https://github.com/llvm/llvm-project/pull/94264


More information about the cfe-commits mailing list