Skip to content

Commit 67a3e45

Browse files
pyricauclaude
andcommitted
Say "Stuck" and "Expected", and name the faulty reference
No word built on "leak" belongs on an object. A leak is one faulty reference that should have been cleared, and everything under it is retained by that single mistake — so "Leaked" on twenty objects points a reader at the twenty rather than at the one thing to fix. "Stuck" says the object's situation without accusing it, and it is the only candidate that asks a question instead of closing one: something is holding this, what? "Expected" says its presence in memory is legitimate at this point in the app's life. No other analyser has a verdict like this to borrow words from, checked before choosing: JProfiler classifies objects by reference type and by age, YourKit by reachability scope, MAT and dotMemory name places rather than objects. What they share is the frame — JProfiler asks whether objects "are still legitimately on the heap or if a faulty reference keeps them alive" — which is where the name for the culprit edge comes from, and it is now what the leaks screen and the KDoc call it. An object nothing reaches any more stays Stuck like any other: it was expected to be gone, and only the collector not having run keeps it here. A fourth value would have made the verdict mean something different in one corner of the window. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent b9f15b4 commit 67a3e45

15 files changed

Lines changed: 180 additions & 130 deletions

File tree

docs/shark-explorer-changelog.md

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,11 @@ uses, without the one for a newly recognized library leak:
3030
See [Take notes](shark-explorer.md#take-notes).
3131
* ✨ Right click ← or → for the list of everywhere that arrow leads, so going back four moves is one
3232
click rather than four.
33-
***Verdict**: whether the object a tab is on leaked `Leaked`, `Needed` or `? Unknown` — is the first
34-
thing the **What it is** panel says, in the same colours a chain uses, and the pencil beside it overrules
35-
the verdict: a reason is required, kept with it, and what you overruled is recorded beside it. Where a
36-
verdict you set contradicts another one, every verdict it disagrees with is listed with the reason it was
37-
given, and keeping yours flips them. The **Leaks** screen follows what you set, since marking an object as
38-
leaked makes it a leak and takes whatever it holds off the list. Kept between runs in
33+
***Verdict**: whether the object a tab is on is stuck in memory `Stuck`, `Expected` or `? Unknown`
34+
— is the first thing the **What it is** panel says, in the same colours a chain uses, and the pencil beside
35+
it overrules the verdict: a reason is required, kept with it, and what you overruled is recorded beside it.
36+
Where a verdict you set contradicts another one, every verdict it disagrees with is listed with the reason
37+
it was given, and keeping yours flips them. The **Leaks** screen follows what you set, since marking an
38+
object as stuck makes it a leak and takes whatever it holds off the list. Kept between runs in
3939
`~/.shark-explorer/leak-statuses`, one file per heap dump.
40-
See [Say what leaked](shark-explorer.md#say-what-leaked).
40+
See [The verdict](shark-explorer.md#the-verdict).

docs/shark-explorer.md

Lines changed: 34 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,8 @@ a few seconds.
7070
Java heap from API 26 to 34, and for those the app offers to fetch them off the device the dump came from.
7171
* **Object list** is the whole dump as a searchable list, and **Starred** keeps the objects you want to
7272
come back to.
73-
* **Whether the object a tab is on leaked is the first thing "What it is" says**, and you can overrule it —
74-
see [Say what leaked](#say-what-leaked).
73+
* **The verdict on the object a tab is on is the first thing "What it is" says**`Stuck`, `Expected` or
74+
`Unknown` — and you can overrule it, see [The verdict](#the-verdict).
7575
* Every location takes a **note**, in markdown, kept between runs — see [Take notes](#take-notes).
7676

7777
## Link to a tab
@@ -149,51 +149,57 @@ Since a `shark://` link names a window, a link written into a note stops working
149149
— see above. Copy one for the tab you want to come back to *while you are writing about it*, and it will
150150
take you there for as long as that window is open.
151151

152-
## Say what leaked
152+
## The verdict
153153

154-
At the top of **What it is**, under the object's name, is the **Verdict** on it — `Leaked`, `Needed`, or
154+
At the top of **What it is**, under the object's name, is the **Verdict** on it — `Stuck`, `Expected`, or
155155
a quiet `? Unknown` — with the reason under it, in the same colours the chain on the left uses. Most objects
156156
in a heap dump are `Unknown`, which is why that one is drawn small: the two that mean something are the ones
157-
worth seeing across the room. This is the same answer a LeakCanary leak trace prints as `Leaking: YES`, `NO`
158-
and `UNKNOWN`, said about the object rather than about the leak — an object *leaked*, and a leak is the
159-
reference still holding it.
157+
worth seeing across the room.
160158

161-
The reason is the whole of the answer, because half of these are about another object: an activity is red
162-
because its own `mDestroyed` is true, and the view under it is red because the activity is. `Activity↑
163-
leaked` is the chain saying so.
159+
`Stuck` says the object should be gone and something is holding it. `Expected` says its being in memory is
160+
legitimate at this point in the app's life. It is the same answer a LeakCanary leak trace prints as
161+
`Leaking: YES`, `NO` and `UNKNOWN`, in words that stop short of calling the object the leak — because
162+
**the leak is the faulty reference**, the one that should have been cleared, and everything under it is stuck
163+
by that single mistake. An object nothing reaches any more is `Stuck` as well: it was expected to be gone,
164+
and only the garbage collector not having run keeps it here. The verdict means the same thing everywhere.
164165

165-
**The pencil beside it** overrules it. Pick one of the three statuses, type why, and **Set the status**:
166+
The reason is the rest of the answer, because half of these are about another object: an activity is red
167+
because its own `mDestroyed` is true, and the view under it is red because the activity is. `Activity↑ is
168+
stuck` is the chain saying so — and it is also how you find the faulty reference, which is the step between
169+
the last `Expected` object and the first `Stuck` one.
170+
171+
**The pencil beside it** overrules the verdict. Pick one of the three, type why, and **Set the verdict**:
166172

167173
* **Your answer wins**, whatever the inspectors said. Overruling is the point — an inspector reads a field,
168174
you read the code, and a cache that is meant to hold what it holds is not something a field can say.
169-
* **The reason is required.** A status with no reason is one nobody who reads your heap dump next — a
170-
colleague, an agent, you in a month — can check, and one of those makes every other status in it worth
175+
* **The reason is required.** A verdict with no reason is one nobody who reads your heap dump next — a
176+
colleague, an agent, you in a month — can check, and one of those makes every other verdict in it worth
171177
less. What you overruled is kept beside your reason rather than thrown away.
172178
* **It reads as yours**, wherever it appears: `set by hand — the cache is bounded, this is fine`, in the
173179
panel and on every chain that runs through the object.
174-
* **Everything a leaked object holds has leaked too, and everything holding a needed one is needed too**, so
175-
a verdict you set changes what the objects around it read as. Which is why setting one is usually enough to
176-
make a whole chain make sense.
180+
* **Everything a stuck object holds is stuck too, and everything holding an expected one is expected too**,
181+
so a verdict you set changes what the objects around it read as. Which is why setting one is usually enough
182+
to make a whole chain make sense.
177183
* **The pencil again** on an object you have already decided about, and **Take it off** to hand it back to
178184
the heap dump.
179185

180186
Because a verdict propagates along the chain, two of them can contradict each other: an object marked as
181-
leaked, holding one marked as needed, cannot both be read off the chain between them. When what you are setting does that, **the window lists every status it disagrees with before writing
182-
anything** — what the object is, which side of yours it is on, the reason it was given, and what it would
183-
become. **Keep this and flip those** keeps yours and sets them to the opposite status, with what they said
184-
kept as part of the new reason; **Undo** leaves the heap dump exactly as it was. Nothing is written until you
185-
pick one.
187+
stuck, holding one marked as expected, cannot both be read off the chain between them. When what you are
188+
setting does that, **the window lists every verdict it disagrees with before writing anything**what the
189+
object is, which side of yours it is on, the reason it was given, and what it would become. **Keep this and
190+
flip those** keeps yours and sets them to the opposite verdict, with what they said kept as part of the new
191+
reason; **Undo** leaves the heap dump exactly as it was. Nothing is written until you pick one.
186192

187-
The statuses live in `~/.shark-explorer/leak-statuses`, one tab separated file per heap dump, with the columns
193+
The verdicts live in `~/.shark-explorer/leak-statuses`, one tab separated file per heap dump, with the columns
188194
named at the top — so they can be read, edited, diffed or pasted into an issue without this app, and they are
189195
there again the next time you open that dump.
190196

191-
**The Leaks screen follows what you set**, because a status changes which objects are leaks and not only how
192-
one of them reads: marking something as leaked makes it a leak, and whatever it holds stops being one — it is
193-
only still in memory because of the object you named, and that is the thing to fix. Marking a leak as needed
194-
takes it off the list. The one thing this costs is that a leak's
195-
fingerprint matches the one LeakCanary reports only while nothing has been set by hand, since the fingerprint
196-
is the stretch of chain your status has just moved.
197+
**The Leaks screen follows what you set**, because a verdict changes which objects are leaks and not only how
198+
one of them reads: marking something as stuck makes it a leak, and whatever it holds stops being one — it is
199+
only still in memory because of the object you named, and that is the thing to fix. Marking a leak as
200+
expected takes it off the list. The one thing this costs is that a leak's fingerprint matches the one
201+
LeakCanary reports only while nothing has been set by hand, since the fingerprint is the stretch of chain your
202+
verdict has just moved.
197203

198204
## Reporting a problem
199205

shark/shark-explorer/AGENTS.md

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,14 @@ found, and the search for every way an object is held runs for the object clicke
6767
question the panels ask has to be measured before it goes in the hover path — `notes/decisions.md` has the
6868
numbers on the biggest dump in the repo, including how long a chain gets there.
6969

70-
## A leaking status set by hand is an argument to every read, never state of the tree
70+
## A verdict set by hand is an argument to every read, never state of the tree
71+
72+
**The window says `Verdict`, `Stuck`, `Expected` and "faulty reference"; the code says `LeakStatus`,
73+
`LEAKING`, `NOT_LEAKING` and `suspectPath`.** That's deliberate — the identifiers match
74+
`shark.LeakTraceObject.LeakingStatus` because they have to agree with Shark, and the words on screen
75+
deliberately avoid "leak" on an object, since a leak is one faulty reference and calling everything under it
76+
leaking points readers at the wrong thing. `LeakStatus.statusText` is the only place the two meet, so change
77+
a word there and nowhere else. `notes/decisions.md` has why each word won.
7178

7279
Someone reading a heap dump can overrule what the inspectors made of an object, and the statuses they set
7380
are a `LeakStatusOverrides` **passed into every question whose answer they change**`summarize`,

shark/shark-explorer/notes/decisions.md

Lines changed: 38 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -841,29 +841,52 @@ a chain reads them here for free.
841841
`Shallow` beside it. It is the one line of the panel that is a judgement rather than a measurement, which is
842842
also what says the pencil beside it is allowed to disagree with it.
843843

844-
**"Leaking" and "Not leaking" became "Leaked" and "Needed"**, `Unknown` staying `Unknown`. LeakCanary's leak
845-
traces say `Leaking: YES / NO / UNKNOWN`, and matching them was the case for keeping the old words — but
846-
calling an *object* leaking reads as that object leaking something as easily as being the thing left behind,
847-
and this repo's own rule is that a leak is a reference rather than an object. `Leaked` is that rule in the
848-
tense: the object leaked, and the leak is the reference still holding it. `Needed` is what an inspector
849-
actually recognizes — something still needs this object.
850-
851-
The first attempt at those two was `Shouldn't be here` and `Meant to be here`, taken from the module's own
852-
prose, and it was rejected on sight for the reason a status has to be read a dozen times down one chain:
853-
**a status is a label, not a sentence.** The header carries the question so the labels don't have to, which
854-
is also why they can be one word each. One `LeakStatus.statusText` is where they live, so the chain, the
855-
panel, the dialog, the checkbox that shades them over the map and the reasons propagated along a chain
856-
(`Activity↓ is needed`) all say the same thing. The identifiers didn't move: `LeakStatus`, `LEAKING`,
857-
`leakStatusesOf` stay Shark's names, because the code is where matching
844+
**"Leaking" and "Not leaking" became `Stuck` and `Expected`**, `Unknown` staying `Unknown`, and **no word
845+
built on "leak" is allowed on an object**. A leak is one faulty reference that should have been cleared, and
846+
everything under it is retained by that one mistake — so `Leaking` or `Leaked` on twenty objects points a
847+
reader at the twenty rather than at the one thing to fix. `Stuck` names the object's situation without
848+
accusing it, and it is the only candidate that asks a question rather than closing one: something is holding
849+
this, what? `Expected` says its presence in memory is legitimate at this point in the app's life. The pair is
850+
deliberately not antonyms — an object in use can't be collected either, so the good side has to answer a
851+
different question than "can it leave".
852+
853+
**No other analyser has a verdict like this**, so there were no words to borrow. Checked: JProfiler
854+
classifies objects by reference type (strongly referenced, retained by soft references) and by age (`Mark
855+
Heap`, then "new" and "old" objects); YourKit by reachability scope (strong, softly, weakly reachable,
856+
unreachable, pending finalization); Eclipse MAT names places rather than objects (*leak suspect*,
857+
*accumulation point*, *keep-alive path*); dotMemory has *key retention paths*. None labels an object leaking,
858+
because none has watched objects or framework inspectors to do it with — they rank by size and leave the
859+
judgement to the reader. What they do share is the frame: JProfiler asks whether objects "are still
860+
legitimately on the heap or if a **faulty reference** keeps them alive", which is where the name for the
861+
culprit edge comes from, and YourKit defines a leak as objects "not needed anymore according to the
862+
application logic".
863+
864+
Two attempts came before this one. `Shouldn't be here` / `Meant to be here` was rejected on sight —
865+
**a verdict is a label, not a sentence**, since it is read a dozen times down one chain. `Leaked` / `Needed`
866+
was rejected for the misdirection above. One `LeakStatus.statusText` is where the words live, so the chain,
867+
the panel, the dialog, the checkbox that shades them over the map and the reasons propagated along a chain
868+
(`Activity↓ is expected`, `Activity↑ is stuck`) all say the same thing. The identifiers didn't move:
869+
`LeakStatus`, `LEAKING`, `leakStatusesOf` stay Shark's names, because the code is where matching
858870
`shark.LeakTraceObject.LeakingStatus` matters.
859871

872+
**The verdict means the same thing on an unreachable object.** A watched object nothing reaches any more is
873+
`Stuck` like any other, even though what keeps it is the collector not having run rather than a faulty
874+
reference: it was expected to be gone. Where it sits on that scale is what the leaks screen's `Unreachable`
875+
section is for, and a fourth value would have made the verdict mean something different in one corner of the
876+
window.
877+
878+
**"Faulty reference" is the name for the culprit**, the reference between the last `Expected` object and the
879+
first `Stuck` one, which is what `LeakGroup.suspectPath` starts at and what the leaks screen names each row
880+
after. Only prose so far — nothing on a chain marks that step yet, and marking it is the change that would
881+
actually put a reader's eye on the reference rather than on the objects.
882+
860883
**A pencil, left of the status, rather than a "Set by hand…" button.** It is what changes the answer, so it
861884
belongs where the eye already is, and a text button pushed the reason onto a second line of a 320dp panel.
862885
Disabled until the statuses have been read off disk, which is the same rule the button had.
863886

864887
**From the last step of the chain when there is one**, and from the object's own reading until the walk up to
865888
the GC roots lands, since the chain's answer is the one with the objects above and below taken into account.
866-
So the panel can say `Unknown` for a beat and then say `Leaked` — the panes filling in, not the window
889+
So the panel can say `Unknown` for a beat and then say `Stuck` — the panes filling in, not the window
867890
changing its mind. Nothing at all for the tab a window opens with: the whole heap dump is no
868891
object of it, and there is nothing to inspect or decide about.
869892

shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/HeapDumpExplorer.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ internal fun HeapDumpExplorer(
212212
* From the last step of the chain when there is one, because that is the status with everything above and
213213
* below the object taken into account, and from the object's own reading until the walk up to the GC roots
214214
* lands — or for good, for an object nothing reaches. So this can say `Unknown` for a beat and then say
215-
* `Leaked`, which is the panes filling in rather than the window changing its mind.
215+
* `Stuck`, which is the panes filling in rather than the window changing its mind.
216216
*
217217
* Nothing for the whole heap dump, which is no object of it: there is nothing to inspect and nothing to
218218
* decide about.

0 commit comments

Comments
 (0)