Make an owner rule code, and hold a screen's views through its window - #2941
Open
pyricau wants to merge 2 commits into
Open
Make an owner rule code, and hold a screen's views through its window#2941pyricau wants to merge 2 commits into
pyricau wants to merge 2 commits into
Conversation
A rule used to be a schema: a class whose instances are owned, plus the fields and virtual references that own them. It said the same thing twice — for a dependency injection singleton the only way to name the owned objects *is* the owner reference, which is what forced the sealed OwnedObjects class in the first place. It also merged every rule's fields into one map with no rule identity, so nothing checked that the field that owns matches the objects it owns; that was sound only by luck. So a rule is now a class taking the graph, like the reference readers beside it, saying which classes can own and which of an owner's references do. Which objects are owned is derived from those references. That deletes OwnedObjects, the four-name ScopedProvider, the merged field maps, the per-class Ownership cache and the name resolution the hot path did per reference — asking whether a reference is a rival is now one hash lookup. Deriving turned out to be a fix, not just a restructuring. Declaring "every View is owned" made a view no owner points at held as a last resort, which made every rival into it count, and Activity.mDecor is null on a live activity more often than not — what holds a decor view is the window. So a whole window's hierarchy was drawn as a flat pile: on large-dump.hprof a PhoneWindow retained 20 KB with 1.6 MB of its own views hanging off the activity beside it, and in a synthetic dump the decor view floated to the root of the tree. Derived, that PhoneWindow retains 1.68 MB and each view nests under its parent. Measured across the eight real heap dumps in the repo: identical object sets, identical total byte counts, and 0 to 53 objects of 34 K to 340 K changed dominator, all of them windows and views. The pass costs 4 ms to 26 ms where naming the owned classes cost 12 ms to 32 ms, of the 1.5 s it takes to open them.
pyricau
force-pushed
the
owner-rules-as-code
branch
from
August 6, 2026 05:11
47066fd to
c4e8739
Compare
The decor view rule was `Activity.mDecor` / `Dialog.mDecor`, and that field is null on a live activity more often than not. `handleResumeActivity` is the only place that writes it, under `r.window == null`, so it is set once per `ActivityClientRecord` rather than once per `Activity` — a screen recreated on a configuration change is resumed, visible, and has a null `mDecor` for the rest of its life. `PhoneWindow.mDecor` has no such gap: every decor view in the seven real dumps here is pointed at by exactly one. Moving the decor view under its window puts a window's 3 MB where the window lands, so the window needs a rule too — 6 of the 16 windows in those dumps were drawn at the top of the tree, held by a `ViewRootImpl` or a `WindowManagerImpl` as well as by their activity. With the second rule every decor view is under its window and every window of a screen that is up is under that screen. That second rule is the one whose owning reference outlives the construct it is about: `Activity.mWindow` is set in `attach` and never cleared and `Dialog.mWindow` is final, so a destroyed activity and a dismissed dialog go on pointing at a window they have nothing left to do with. Owning it anyway took the window of a leaked screen away from whatever else held it, and turned two leaks into one on the list. So it owns through `mWindow` and asks whether the screen is up of another reference, one the framework does drop — the self-clearing kind every other rule owns by, borrowed from beside the one this rule owns through. A dialog is dropped from its own `mDecor`, which `show` sets and `dismiss` nulls; an activity is dropped from `ActivityThread.mActivities`, which `handleDestroyActivity` removes the record from and which the activity rule already owns activities by. No rule reads the state of an object. Bytes are conserved on all eight dumps in the repo, no object joins or leaves the tree, and 0 to 16 objects of 34 K to 340 K move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2937. The
OwnerRuleschema grew a second vocabulary when the dependency injection rule landed — a sealedOwnedObjectsclass and a four-nameScopedProvider— and that turned out to be a symptom rather than an inconvenience.What was wrong with the schema
It said the same thing twice. Every rule named a class whose instances are owned and the references that own them. For a DI singleton the only way to name the owned objects is the owner reference —
DoubleCheck.instance— so the sealed class existed to express a tautology.It had no room for "and not this object".
ScopedProvider.uninitializedHolderClassNameanduninitializedFieldNamewere a sub-vocabulary for one exclusion that in code isheldObjectId != provider.uninitializedObjectId.It lost which rule a field belonged to.
ownerFieldsByClassName()merged every rule's fields into one map and every rule's virtual class names into one set, andOwnershipcarried no rule identity — so nothing checked that the field that owns matches the objects it owns. Sound today only by luck: the next virtual reference reader whose source class is also aViewGroupwould have silently made thatViewGroupown what the new reader reads.What a rule is now
A class taking the graph, resolving its classes once and answering off the index — the same shape
ViewChildReferenceReaderandRunningActivityReferenceReaderalready have. Two members:Which objects are owned is derived from what those report. Deleted:
OwnedObjects,ScopedProvider's two sentinel fields, the merged field and virtual-class maps, the per-classOwnershipcache, and the lazy name resolution the hot path did for every reference into an owned object. Asking whether a reference is a rival is now one hash lookup into a single map.Consumers are unchanged apart from passing an object id where they passed an
Ownership.Deriving turned out to be a fix
I expected this to be behaviour-preserving and captured a signature of every node of every real dump in the repo beforehand to prove it. It isn't, and the difference is a bug the schema was hiding.
Declaring "every
Viewis owned" meant a view no owner points at was still owned, so it was held as a last resort, so every rival into it counted. AndActivity.mDecoris null on a live activity more often than not — measured null for the only activity ofcompose_leak.hprof, two of the three inleak_asynctask_m.hprof, one of the two ingcroot_unknown_object.hprofand two of the four inlarge-dump.hprof, because what holds a decor view is the window. So the ordinary case was: no owner reaches the decor view, the whole hierarchy becomes last-resort held, and the window's views are drawn as a flat pile.On
large-dump.hprofaPhoneWindowretained 20 KB with 1.6 MB of its own views hanging off the activity beside it. Derived, thatPhoneWindowretains 1.68 MB and each layout layer nests under the one above it. In the new synthetic fixture the old code floats the decor view all the way to"Whole heap dump"— I ran the new test against the old rule code to confirm it fails there.Measurements
Signature of every node — id, retained size, dominator, dominator kind — over the eight real dumps, before and after:
unloaded_classes-stripped.hprof(5.5 GB of retained heap, 314 K objects) is byte-for-byte identical. The two nodes that vanish fromleak_asynctask_m.hprofare synthetic group nodes, not objects: a flat pile of same-class views needed grouping and a nested hierarchy doesn't.Cost of the pass, measured per dump: 4 ms to 26 ms derived against 12 ms to 32 ms declared, of the ~1.5 s it takes to open these dumps. Deriving reads a record per possible owner where declaring read none, and buys back the name resolution the hot path was doing per reference.
notes/dominator-tree.mdrecords all of it.No changelog entry: the explorer's log is still a single "Initial release".
Second commit: hold a decor view through its window, and the window through its screen
Which was #2943 until it turned out to be one change with the first commit rather than a follow-up.
The question it answers: is there always a window when there's a decor view, and if so, should the
rule be
PhoneWindow.mDecorrather thanActivity.mDecor?Is there always a window?
Yes, for a decor view: every one in the seven real dumps in the repo is pointed at by exactly one
PhoneWindow.mDecor(1 of 1 incompose_leak.hprof, 3 of 3 inleak_asynctask_m.hprof, and so on).Not for a root view, which is the distinction that matters: 14 of the 15 root views in
compose_leak.hprof, 9 of the 10 inleak_asynctask_m.hprofand 22 of the 23 ingcroot_unknown_object.hprofhave no window anywhere. They'reToastviews,PopupWindow.mContentView,TextView$MagnifierViews,Toolbar.mNavButtonViewimage buttons and fragment roots, each held the ordinary way by whatever made it. So the rule is window → decor view, and the rest need nothing.Why
Activity.mDecorhad to goActivityThread.handleResumeActivityis the only place that writes it, guarded byif (r.window == null && !a.mFinished && willBeVisible)— so it is set once perActivityClientRecord, not once perActivity. A screen recreated on a configuration change is resumed, visible, and has a nullmDecorfor the rest of its life. It also inverted the nesting where it was set: inleak_asynctask_m.hprofaPhoneWindowwas drawn under its ownDecorView, throughDecorView.this$0.Why the window then needs a rule of its own
A window has 6 to 9 referrers in these dumps. Most are its own inner classes and its decor's, but the external ones —
WindowManagerImpl.mParentWindow,ActivityThread$ActivityClientRecord.window,DecorView.mWindowreached from aViewRootImpl— put 6 of the 16 windows at the top of the tree. Moving the decor view under the window makes that worse than it was, because the hierarchy now floats with it:gcroot_unknown_object.hprof'sPhoneWindowwent from 14 KB at the root to 3.09 MB there,large-dump.hprof's from 12 KB to 2.14 MB. WithActivity.mWindow/Dialog.mWindowowning it, every decor view in every dump is under its window and every window of a screen that is up is under that screen.The rule whose owning reference outlives what it is about
Activity.mWindowis set inattachand never cleared, andDialog.mWindowis final, so a destroyed activity and a dismissed dialog go on pointing at a window they have nothing left to do with. Owning it regardless took the window of a leaked screen away from whatever else was holding it and turned two leaks into one on the list — the reference that would still have been there after the activity was fixed stopped being reported.HeapLeaksTestcovers that.So
ActivityWindowRuleowns throughmWindowand asks whether the screen is up of another reference, one the framework does drop — the same self-clearing signal every other rule owns by, borrowed from beside the one this rule owns through:mDecor, whichshowsets anddismissnulls.ActivityThread.mActivities, whichhandleDestroyActivityremoves the record from — and whichRunningActivityRulealready owns activities by, so the two rules now share oneRunningActivityReferenceReader.No rule reads the state of an object. The first draft of this one read
Activity.mDestroyed; swapping it for the map gives byte-identical trees and leak lists on all eight real dumps, withmActivities.sizematching the count of non-destroyed activities in every one of them. They also fail in opposite directions, which is the reason to prefer the map: anmDestroyeda dump doesn't carry reads as a screen that is up and keeps ownership, where anmActivitiesthis can't read reads as a thread running nothing and gives ownership up, which is the fallback every other rule already has.notes/dominator-tree.mdrecords this as the test to apply to a new rule: ask which reference the framework drops, and own by that one or read it.Conserved
Bytes conserved on all eight dumps, no object joins or leaves the tree, 0 to 16 objects of 34 K to 340 K move.
shark-explorer-core:checkandshark-explorer-app:checkpass.🤖 Generated with Claude Code