fix(native): release resolved-style entries, and compute a derived observable once per change - #2
Draft
YevheniiKotyrlo wants to merge 6 commits into
Draft
Conversation
…mounts The resolved-style cache is never released. Not on unmount, not when an entry is superseded, not ever — a plain `<View className="x" />` that mounts and unmounts leaves its entry in the map for the life of the process, and every unmounted component stays reachable from every observable it read, through the `run` closure that holds its `setState`. The release was written and never reached. Three things had to be true for it to work and none of them was. **The dependency graph is one-directional.** `observable.get(effect)` records the observable's view of its subscriber and not the subscriber's view of what it reads, so `effect.observers` is always empty. `cleanupEffect` walks exactly that set to detach a subscriber from everything it read, so it has always iterated nothing: the whole cleanup path is dead code. Recording the reverse edge in `get` is what makes it live. **Both effects shared one dependency Set** — "to improve memory usage", one allocation per component. It also made detaching unexpressible: `cleanupEffect` removes the effect it was handed from each observable and then clears the shared set, so the sibling stays registered on every observable it ever read. Each effect now owns its set and both are cleaned. **The observable was never told.** An entry can only be dropped at the moment its last observer leaves, which is a fact only the detaching code knows. `Effect` gains an optional `cleanup`, `cleanupEffect` calls it after detaching, and the supersede path passes both effects rather than one. `family` gains an optional `maxSize`, and `stylesFamily` takes one. That is a backstop rather than the fix — the release is what keeps the cache proportional to what is mounted — and it evicts the least recently READ, because the workload that would fill it is a churn of single-use keys beside a small set read every render, where insertion order discards exactly the entries worth keeping. Renewing on a hit costs ~23ns against a ~265ns key derivation. Eviction is safe by construction: a miss re-derives from the rules the caller brought. Measured, a component rendering with an inline `vars()` — which returns a fresh object per call, so each render mints a new key: 13 entries over 12 renders before, 1 after, 0 once it unmounts. Six tests, each red first and each proven red again with the fix reverted.
…elease by identity
Follow-up to the commit before it, which recorded the reverse edge into the wrong
Set. Two reviewers with opposing lenses found the same cause independently.
`observable()` gave its internal effect the SAME Set object as the observable's
subscriber list, so recording `effect.observers.add(obs)` put a source observable
into the derived observable's SUBSCRIBER list — where `notify` walks it. It
typechecks because `Observable` structurally satisfies `Effect`.
Two measured consequences. An entry reading `vw`, an undefined `var()`, or a
`:root` variable under `prefers-color-scheme` could never reach zero observers,
so the release did nothing for the commonest real styles — the previous commit's
own measurement inverts with one extra declaration: `.churn { color: var(--x) }`
gave 13 entries before and 1 after, `.churn { width: 50vw; color: var(--x) }`
gave 13 and 13. And `w-[50vw] animate-spin` plus a dimension change recursed to
`RangeError: Maximum call stack size exceeded`, because the keyframes observable
notifies unconditionally while the styles observable never compares equal.
Three changes:
- The internal effect gets its OWN dependency set, leaving `obs.observers` a pure
subscriber list. Everything above is downstream of this one line.
- `cleanup` drops the reverse edge as well as the forward one. Leaving it meant a
component that superseded its own entry still listed it as a dependency, so its
unmount walked back and released an entry another component had since joined —
reproduced directly: A supersedes, B joins the old entry, A unmounts, B's live
entry is destroyed.
- The release is by IDENTITY. `family` gains `deleteIf`, because the entry closes
over the hash it was created under and that hash may since map to a different
observable, after an eviction and a rebuild or after the stale-reference shape
above. Deleting by hash alone destroys whatever took the key.
Eleven more tests: the release for an entry that reads another observable (a
viewport unit, an undefined variable, a dark-mode root variable), that such an
entry stays reactive while mounted, that a root-variable change does not notify
unrelated `colorScheme` subscribers, the animated-viewport-unit crash, the two
identity cases, and `deleteIf` pinned directly.
Reverting the per-effect Set turns 5 of 8 red. The reverse-edge drop and the
identity check are individually redundant for the stale case — either alone
covers it — so `deleteIf` is pinned on `family` rather than through a revert that
would not demonstrate it.
… per read `getStyledProps` costs 1952ns per call and 488ns after this — 4.0x, on the path every styled element pays on every render. `observable()`'s `get` recomputed whenever `didInit` was falsy, and nothing ever set it for a DERIVED observable: only the static-init branch and `set` did. So a computed value was recomputed per read rather than per change. For the resolved-style cache that is `calculateProps` — the whole style resolution — on every render of every styled element, which is precisely the work the cache exists to avoid. Latching it after the first compute is the whole change. Recomputation on CHANGE is a different path and is untouched: a dependency notifies, `effect.run` re-runs the read function and re-assigns `value`, and subscribers are notified from there. The memo only removes the redundant work between those. Measured with the library's own entry points rather than a render, because an end-to-end bench is dominated by the test renderer — its run-to-run spread was wider than the entire contribution being measured. `getStyledProps` is stable to about 5% across nine repetitions either side. Three tests: a derived observable computes once across twenty reads, it still recomputes when a dependency changes, and a static observable is unaffected. The first is red before this at 21 computes for 21 reads.
…ns again
A batch is a `Set`, and iterating one does not revisit a member it has already
passed. So an effect notified a SECOND time during a drain — because a derived
observable it depends on recomputed after the effect ran — was dropped.
That was survivable while every `get` recomputed: whenever the effect ran it
pulled fresh values out of its dependencies. It is not survivable now that a
derived observable memoises, because the effect reads the value its dependency
held BEFORE the recompute and nothing runs it again. The stale value is
permanent, not a transient glitch.
The reachable shape is `vw -> { stylesObs, rootVariables } -> stylesObs`: a style
that reaches a viewport unit directly through the unit resolver AND again through
a root variable whose media query tests a width. Ordinary CSS:
@media (prefers-color-scheme: dark) and (min-width: 600px) { :root { --gap: 16 } }
:root { --gap: 8 }
.box { width: 50vw; padding: var(--gap) }
After a resize the box keeps `padding: 8` until the next viewport change — and
because `stylesFamily` hands one memoised object to every consumer, a component
mounted after the rotation gets the stale value too.
Both drains — the `Dimensions` change handler and `StyleCollection.inject` — now
call one exported `drainObservableBatch`, which removes a member BEFORE running
it so a re-notification re-enqueues rather than landing on an entry the iteration
has passed.
Two tests. The diamond is mutation-proven: with the previous `for (const effect
of batch)` it observes `2,10` — the derived value from before the change — and
`2,20` with the work list. The memo suite gains a conditional-dependency test
(the first compute takes a branch that never reads B, and opening the gate still
picks up B's CURRENT value, because `effect.run` registers and pulls in the same
pass), and its change test now pins the recompute COUNT rather than asserting it
grew — the loose form was satisfied by the per-read recompute being removed, so
it passed on both sides and guarded nothing.
…a cached entry `nativeStyleMapping` drains keys OUT of the resolved style object and writes them onto real props — `delete source[key]`, then `props[path] = value`. It mutates. That was harmless while every read of the styles observable recomputed: each render got a fresh object and the mutation died with it. The memo makes the object it mutates the CACHED one, shared by every consumer of that entry and surviving every render. It is idempotent — the second pass finds the key already drained and continues — and every shape measures correct. But nothing enforced that, and "it happens to be a no-op" is not a property to leave resting on an accident while the memo makes it load-bearing. Two tests, on the one shipped component with a `target: false` mapping: ten re-renders read the same mapped value, and a second component joining the same cache entry sees it too. A drain that stopped being idempotent would surface as a MISSING prop on the second consumer rather than a wrong one, which is the harder failure to notice.
`get` registered the caller's effect BEFORE running the read function, so a compute that threw left a subscriber attached to an observable that had never initialised — on the observer list, owed nothing, and counted by every question that asks whether anything observes this. `resolve` throws on an unknown function, so the shape is reachable rather than theoretical. Computing first makes the failure leave no trace: the throw propagates to the caller and the observable is exactly as it was before the call. The reverse edge moves with it, so a failed read cannot leave a dangling dependency either. It also closes the one residual an exhaustive review found in the memo two commits back. Over 813,615 no-throw operation sequences that review recorded zero divergences from the pre-memo behaviour and zero firings of the guard the memo was thought to supersede; every firing it could produce had the same shape — a read that threw, registering an observer, followed by a later successful compute. With the registration moved, that sequence cannot arise. Two tests: a read that throws leaves both directions of the graph untouched and the observable still usable once the condition clears, and a successful read still registers.
YevheniiKotyrlo
changed the title
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
1 participant
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.
Top layer of a stack: the six commits that are not part of nativewind#434, with a clean diff.
The submission is upstream: nativewind#436 — review and comment there. That PR has to target
mainand therefore carries nativewind#434's commit as its first, because GitHub's stacked pull requests do not support cross-fork stacks. This PR is the review surface for the difference: basefix/mapping-config-identity(#1), so the diff here is exactly the six commits — 14 files.Nothing is merged in this fork.