Read an evicting cache as an evicting cache, for the caches an app really has

docs/changelog.md

@@ -95,6 +95,7 @@ Releases before 2.8.1 predate these markers.
     * ⚠️ `ThrowingCancelableFileSourceProvider` is replaced by `CancelableSourceProvider`, which wraps any `DualSourceProvider` instead of opening a `File` itself. The class it replaces opened its own `FileInputStream` channel, so it missed everything `FileSourceProvider` does — positional reads that several threads can make at once, a pool of scratch arrays, and opening the file up front so a caller can delete it and keep reading — and it couldn't be composed with any other source. Cancelling a heap analysis mostly doesn't need it at all now: pass a `CancelSignal` when opening the graph and the source is wrapped for you. It's still the way to cancel work that reads a heap dump without opening a graph on it, like `HprofPrimitiveArrayStripper` and `HprofDeobfuscator`.
     * ⚠️ `HeapGraph` has a new `cancelSignal` property, `HprofHeapGraph.openHeapGraph()` and `HprofIndex.indexRecordsOf()` take a `cancelSignal` parameter that defaults to `CancelSignal.NEVER`, and `HprofIndex` carries the signal to the graphs it opens.
     * 🔀 `HeapAnalyzer.analyze()` throws `CanceledException` when a `CancelSignal` stops it, instead of returning a `HeapAnalysisFailure` wrapping it. An analysis that was asked to stop didn't fail, and callers had to sniff `HeapAnalysisFailure.exception.cause` to tell the two apart.
+* ✨ `shark.MapEntryReader` reads a map instance of a heap dump as the entries an app put in it, handing back a `shark.HeapMapEntry` per entry: the node the map holds it in, plus its key and value. It covers the maps Shark's reference readers already cover — `HashMap`, `LinkedHashMap` and `ConcurrentHashMap`, in whichever of the OpenJDK and Apache Harmony implementations the heap dump was written by — and is built out of them, so which fields to read is answered once. Those readers turn a map into `map["key"]` references, leaving the node out because a leak trace doesn't want it; this is the same walk for a tool that has something to say about the node itself, such as that a cache's entry holds its value weakly.
 * 🐛 Canceling a heap analysis running in a separate process (`leakcanary-android-process`) stored a `HeapAnalysisFailure` reading *"Analysis canceled"* in LeakCanary's database, so an analysis nobody asked to finish showed up in the LeakCanary activity as a failed one. Nothing is stored for a canceled analysis now.
 * ⚠️ The `shark` artifact no longer declares a dependency on `kotlinx-coroutines-core`. It was listed as an `implementation` dependency, which put it in the POM at runtime scope, so every consumer of `shark` resolved coroutines 1.7.3 — but no Shark module has ever used it. Nothing needs to change unless you were relying on `shark` to bring coroutines in.
 * 🔀 Curtains 1.2.4 → 1.2.5, which reports Compose popup windows as `POPUP_WINDOW` instead of `UNKNOWN`. `RootViewWatcher.WindowTypeFilter` doesn't expect popup windows to become weakly reachable when they're detached, because Android widgets keep detached popup instances around, so a Compose popup that stays in memory after being detached is no longer reported as a leak.

shark/shark-explorer/notes/dominator-tree.md

@@ -187,7 +187,8 @@ them apart, so it hands the bytes to the root — which is what `ReachabilityStr
 
 The fix for the above is to stop treating a cache's reference as retaining. `CACHE` ranks between
 `STRONG` and `SOFT`, and `ReferenceStrengthReader.CACHE_FIELDS_BY_CLASS_NAME` is the curated list of
-fields it applies to — one entry today, `coil3.memory.RealStrongMemoryCache$InternalValue.image`.
+fields it applies to — one entry today, `coil3.memory.RealStrongMemoryCache$InternalValue.image`. That
+list is half of it; the caches whose values no class name can name are the subsection below.
 
 The mechanics fall out of the weak reference machinery that was already there, because the rule the
 explorer wants is exactly the rule for a weak reference: **the target's strength decides**. A weakening
@@ -210,6 +211,32 @@ Two things to know before adding an entry to that list:
   `InternalValue.image` leaves the map, the entries and the size bookkeeping strongly held by the cache,
   where they belong, and moves only the images.
 
+### A cache that wraps its values in nothing: `CachedMapValues`
+
+The caches an app is most likely to have are the ones a class name can't describe. `android.util.LruCache`,
+Picasso's and Glide's all keep what they cache in a `java.util.HashMap`, so the only thing between the
+cache and the value is a `HashMap$Node`, a class every map in the dump shares: weakening its `value` by
+class name would weaken every map there is, and weakening the cache's map field would take the table, the
+entries and the keys down with it. So which entries are a cache's is read off the heap dump instead —
+`CachedMapValues` reads the map of each cache listed in it through `shark.MapEntryReader` — the entry
+level half of the walk Shark's own map readers make, which hands back the node as well as the key and the
+value — and remembers which node holds which value, one pass over the classes and one over the instances,
+30 to 45 ms on `large-dump.hprof`.
+
+**Two references to drop per value, not one**, and this is the part that breaks silently:
+`DataStructureReferenceReader` reads a map as the entries you put in it, so a cache's map points straight
+at each value as well as through the node holding it. Dropping only the node's leaves the value strongly
+held by the map, which is the cache holding it after all — the weakening then changes nothing and the only
+symptom is a byte count that didn't move.
+
+Measured on `large-dump.hprof`: 3.9 MB moves out of `STRONG` and reads as the Picasso cache it sits in,
+13% of everything that dump held strongly. That cache holds four bitmaps — a 760×1262 and three 126×126,
+4,026,992 B of pixels between them — and **two of them move**, 3,899,984 B: the big one and one of the
+small ones, which nothing else holds. The other two stay `STRONG` under what shows them, which is the rule
+working rather than a gap in it. Nothing moves for `android.util.LruCache` on that dump: all fourteen
+instances of it there are the framework's or a library's own, holding prepared statements and typefaces
+rather than images.
+
 ## An owner beats a bystander: the `OwnerReferences` rule
 
 A view that's part of a hierarchy is held by its parent, and a dominator tree that doesn't know that

shark/shark-explorer/shark-explorer-core/src/main/java/shark/explorer/CachedMapValues.kt

@@ -0,0 +1,207 @@
+package shark.explorer
+
+import androidx.collection.LongSet
+import androidx.collection.MutableLongLongMap
+import androidx.collection.MutableLongObjectMap
+import androidx.collection.MutableLongSet
+import shark.HeapGraph
+import shark.HeapObject
+import shark.HeapObject.HeapInstance
+import shark.MapEntryReader
+import shark.SharkLog
+import shark.ValueHolder
+
+/**
+ * Which map entries of a heap dump hold a value a cache holds, so that the value reads as
+ * [ReachabilityStrength.CACHE] while the map, its entries and its keys stay strongly held by the cache.
+ *
+ * The caches [ReferenceStrengthReader] already knows about wrap each cached value in a class of their own
+ * — Coil's `RealStrongMemoryCache$InternalValue` — so naming that class and the field is the whole of it.
+ * The ones here keep their values in a `java.util.HashMap`, where the only thing between the cache and
+ * what it caches is a `HashMap$Node`, a class every map of the heap dump shares. Weakening its `value`
+ * field by class name would weaken every map there is, so which entries are a cache's is read off the
+ * heap dump instead: this reads the map of each cache through [MapEntryReader] and remembers which node
+ * holds which value.
+ *
+ * Cutting there rather than at the map keeps the cache's own bookkeeping where [ReferenceStrengthReader]
+ * keeps it — strongly held, its bytes attributed to the cache — since weakening the map would take the
+ * table, the entries and the keys down with it.
+ *
+ * Two references to drop rather than one, though: [DataStructureReferenceReader] reads a map as the
+ * entries you put in it, so the map points straight at each value as well as through the entry holding it.
+ * Both go, and only the entry's is handed back as the weakening one, so that a cached value is held one
+ * way and weakly.
+ */
+internal class CachedMapValues(private val graph: HeapGraph) {
+
+  /**
+   * The cached values of the heap dump, by what points at them.
+   *
+   * Built on first use rather than in a constructor, and as a whole object assigned at once, so that a
+   * read given up on half way is retried rather than remembered — the rule for every index here, see the
+   * shark-explorer AGENTS guide. Empty for a heap dump with none of these caches in it, which is most.
+   */
+  private val cached: CachedValues by lazy { readCacheMaps() }
+
+  /**
+   * The value [source] holds as an entry of a cache, or [ValueHolder.NULL_REFERENCE] for anything else —
+   * which is nearly every object of a heap dump, and every object of one with no such cache in it.
+   */
+  fun cachedValueIdOf(source: HeapObject): Long = if (source is HeapInstance) {
+    cached.cachedValueIdByEntryId.getOrDefault(source.objectId, ValueHolder.NULL_REFERENCE)
+  } else {
+    ValueHolder.NULL_REFERENCE
+  }
+
+  /**
+   * Which of the objects [source] points at read as cached, and so as references that don't retain: the
+   * one value an entry holds, and every value of a cache's map.
+   *
+   * The map is in here because it points straight at each of its values as well —
+   * [DataStructureReferenceReader] reads a map as the entries you put in it, which is a second reference
+   * to the same value from a second object. Dropping only the entry's would leave the value strongly held
+   * by the map, which is the cache holding it after all.
+   */
+  fun cachedValueIdsOf(source: HeapObject): LongSet = if (source is HeapInstance) {
+    cached.cachedValueIdsBySourceId[source.objectId] ?: NOTHING_CACHED
+  } else {
+    NOTHING_CACHED
+  }
+
+  /**
+   * How the maps of this heap dump are laid out, which is Shark's answer rather than one of our own:
+   * which class holds a map's table and what its nodes call their fields differs between the OpenJDK
+   * implementation and the Apache Harmony one Android shipped before it, and [MapEntryReader] is that
+   * question already answered — see its KDoc for why it hands back nodes when a leak trace doesn't.
+   */
+  private val mapEntryReader by lazy { MapEntryReader.createFor(graph) }
+
+  /**
+   * Walks the map of every cache of [CACHE_MAP_FIELDS_BY_CLASS_NAME], which takes one pass over the
+   * classes of the heap dump, one over its instances, and a read per entry of the caches it found.
+   */
+  private fun readCacheMaps(): CachedValues {
+    val cachedValues = CachedValues(MutableLongLongMap(), MutableLongObjectMap())
+    val mapFieldNameByClassId = mapFieldNameByCacheClassId()
+    if (mapFieldNameByClassId.isEmpty()) {
+      return cachedValues
+    }
+    graph.instances.forEach { instance ->
+      mapFieldNameByClassId[instance.instanceClassId]?.let { mapFieldName ->
+        readEntriesOf(instance, mapFieldName, cachedValues)
+      }
+    }
+    return cachedValues
+  }
+
+  /**
+   * Which field holds the map of a cache, by the class object id of every class that is one — the listed
+   * classes and their subclasses, since `LruResourceCache` is how Glide's `LruCache` is used.
+   *
+   * Reads no object record: a class's name and its superclass both come from the heap dump index.
+   */
+  private fun mapFieldNameByCacheClassId(): MutableLongObjectMap<String> {
+    val mapFieldNameByClassId = MutableLongObjectMap<String>()
+    graph.classes.forEach { heapClass ->
+      // Most derived class first, so that a subclass listed with a field of its own wins over the one it
+      // inherits.
+      heapClass.classHierarchy
+        .firstNotNullOfOrNull { CACHE_MAP_FIELDS_BY_CLASS_NAME[it.name] }
+        ?.let { mapFieldName -> mapFieldNameByClassId[heapClass.objectId] = mapFieldName }
+    }
+    return mapFieldNameByClassId
+  }
+
+  /**
+   * Reads the entries of [cache]'s map into [cachedValues].
+   *
+   * A map caught mid-insertion reads as what it holds rather than as what it is about to, since the walk
+   * follows the chain each bucket starts rather than trusting the entry count.
+   */
+  private fun readEntriesOf(
+    cache: HeapInstance,
+    mapFieldName: String,
+    cachedValues: CachedValues
+  ) {
+    val map = cache.readFields()
+      .firstOrNull { it.name == mapFieldName }
+      ?.valueAsInstance
+      ?: return
+    val entries = mapEntryReader.readEntriesOf(map)
+    if (entries == null) {
+      SharkLog.d {
+        "${cache.instanceClassSimpleName} keeps its entries in ${map.instanceClassSimpleName}, which " +
+          "Shark has no map reader for, so nothing of it reads as cached"
+      }
+      return
+    }
+    entries.forEach { entry ->
+      val valueId = entry.value.asNonNullObjectId ?: return@forEach
+      if (graph.objectExists(valueId)) {
+        cachedValues.add(entryId = entry.instance.objectId, mapId = map.objectId, valueId = valueId)
+      }
+    }
+  }
+
+  /**
+   * What the two questions above are answered from, filled in as the caches are walked and read together
+   * once the walk is done.
+   */
+  private class CachedValues(
+    /** What each cache entry points at: the one reference out of it that doesn't retain. */
+    val cachedValueIdByEntryId: MutableLongLongMap,
+    /** Every cached value each cache entry and each cache map points at. */
+    val cachedValueIdsBySourceId: MutableLongObjectMap<MutableLongSet>
+  ) {
+
+    fun add(
+      entryId: Long,
+      mapId: Long,
+      valueId: Long
+    ) {
+      cachedValueIdByEntryId[entryId] = valueId
+      cachedValueIdsBySourceId.getOrPut(entryId) { MutableLongSet() } += valueId
+      cachedValueIdsBySourceId.getOrPut(mapId) { MutableLongSet() } += valueId
+    }
+  }
+
+  companion object {
+    /**
+     * The field a cache keeps its `java.util.HashMap` of entries in, by the name of the class declaring
+     * it, subclasses included.
+     *
+     * Curated to the same bar as the cache field list in [ReferenceStrengthReader], and its other half: a
+     * class belongs in one list or the other depending only on whether the cache wraps each value in a
+     * class of its own. Add an entry only against a heap dump that has the cache in it.
+     */
+    private val CACHE_MAP_FIELDS_BY_CLASS_NAME = mapOf(
+      // The framework's own LRU, size bounded and evicting on every put, and what an app reaches for to
+      // cache its own bitmaps. In here for the app's own caches rather than for a measurement of its
+      // own: every one of the fourteen instances of it in `large-dump.hprof` is the framework's or a
+      // library's — nine SQLite prepared statement caches, a job cache, a typeface cache, two empty ones
+      // — so what it moves on that dump is bookkeeping and not pixels. The bitmaps that dump does hold
+      // in a cache are Picasso's below, which is a class of its own and no subclass of this.
+      "android.util.LruCache" to "map",
+      // Picasso's memory cache of decoded bitmaps, evicted over the maximum size and on `onTrimMemory`.
+      // `large-dump.hprof` has one holding four bitmaps, a 760×1262 and three 126×126. Two of them are
+      // held by nothing else and move, 3,899,984 B, which sat at the top of the tree because a cache was
+      // all there was to blame; the other two stay strong under what shows them.
+      "com.squareup.picasso.LruCache" to "map",
+      // Glide's, which `LruResourceCache` is: the decoded resources of images nothing is displaying any
+      // more, evicted over the maximum size and on `onTrimMemory`. What is displaying one holds it
+      // through an `EngineResource` of its own, so an image in use is attributed to the view showing it
+      // and only what is idle is left under the cache.
+      "com.bumptech.glide.util.LruCache" to "cache"
+    )
+
+    /** Read by every object of a heap dump with no cache in it, so one set rather than one per object. */
+    private val NOTHING_CACHED = MutableLongSet(initialCapacity = 0)
+
+    /**
+     * What the entry of a cache calls the field this weakens, for the panel that names it. Every cache
+     * above keeps its entries in a `java.util.HashMap` or a `LinkedHashMap`, whose nodes call it `value`;
+     * a `ConcurrentHashMap`, which [MapEntryReader] also reads, calls it `val`.
+     */
+    const val VALUE_FIELD_NAME = "value"
+  }
+}