From 152b5ad6d669b1e248cddc34a57c90f4db2e8aae Mon Sep 17 00:00:00 2001 From: Yevhenii Date: Wed, 19 Aug 2026 20:12:57 +0300 Subject: [PATCH] fix(native): one resolved-style cache key per distinct input set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `generateStateHash` keys the resolved-style cache on weak-key identity, so it shares only when equal inputs arrive as one object. Three defects break that. **The config is minted per instance.** `styled()` derives it once at module scope; `useCssElement` derives one per instance from a module constant — same value, fresh identity, so N identical elements each hold their own entry, sorted rule array and observable. `getRuleVariation`'s rule clone is keyed on that identity too, so it was per element rather than per mapping. Deriving through `weakFamily` keys the config on the mapping. A mapping that is not an object is refused by name rather than through the `Invalid value used as weak map key` a primitive would otherwise raise from inside `reactivity`; the predicate is narrower than that WeakMap contract, since a function is a valid weak key and still not a mapping. **`family` and `weakFamily` cache on truthiness.** A factory returning `0` is re-run on every lookup, and `hashKeyFamily` hands out `hashKeyCount++` — so the first weak key hashed never settles. Of twelve call sites it is the only one whose result can be falsy, so caching on presence changes nothing else. **The key is lossy.** Folding N numbers into one and printing it base-36 is a digest, and `family` returns the entry it holds while ignoring the rules the second caller brought — so a collision renders one element's styles on another. Over one config plus one rule, the commonest shape, the fold collides 31.99% of the time. Sorting and joining is exact and keeps order-independence. The numbers are collected into a `Float64Array` so the sort is native: ordering them with `Array.prototype.sort` needs a comparator, and that callback is most of what ordering costs on Hermes — 1.10x the fold at the median key count against 1.64x. Float64 rather than Int32 because the key counter is unbounded and an int32 wraps silently at 2^31. Two inherited lines go with the fold: `generateStateHash`'s empty-string sentinel — unreachable, and collidable once the key is a join — together with the two parameters no caller passes, and the loop's `if (!key) continue`, which erased a member from a value whose whole point is that it erases nothing. `family` gains `size()` so the invariant is assertable. It reaches `rootVariables` and `universalVariables`, which are `family` instances published through `./native-internal` — additive, beside the `delete` and `clear` already on them. 50 identical elements: 50 entries before, 1 after. A 150-control screen on a physical Android device: 1785 → 270-271. Re-entering a screen grew the cache without limit (2, 4, 6 …) and is now flat; entries are still never released on unmount, which is a separate defect. 19 tests across five files, each red first. --- src/__tests__/native/config-identity.test.ts | 121 ++++++++++++++++++ src/__tests__/native/family.test.ts | 42 ++++++ src/__tests__/native/state-hash.test.ts | 73 +++++++++++ .../native/style-cache-bounds.test.tsx | 104 +++++++++++++++ .../native/style-cache-sharing.test.tsx | 43 +++++++ src/native/react/rules.ts | 67 +++++----- src/native/react/useNativeCss.ts | 39 +++++- src/native/reactivity.ts | 7 +- 8 files changed, 459 insertions(+), 37 deletions(-) create mode 100644 src/__tests__/native/config-identity.test.ts create mode 100644 src/__tests__/native/family.test.ts create mode 100644 src/__tests__/native/state-hash.test.ts create mode 100644 src/__tests__/native/style-cache-bounds.test.tsx create mode 100644 src/__tests__/native/style-cache-sharing.test.tsx diff --git a/src/__tests__/native/config-identity.test.ts b/src/__tests__/native/config-identity.test.ts new file mode 100644 index 00000000..703aae80 --- /dev/null +++ b/src/__tests__/native/config-identity.test.ts @@ -0,0 +1,121 @@ +import { ScrollView as RNScrollView, View as RNView } from "react-native"; + +import { generateStateHash } from "../../native/react/rules"; +import { + mappingToConfig, + type ComponentState, + type Config, +} from "../../native/react/useNativeCss"; +import { + VAR_SYMBOL, + type Effect, + type VariableContextValue, +} from "../../native/reactivity"; +import type { StyledConfiguration } from "../../runtime.types"; + +/** + * The resolved-style cache is keyed by `generateStateHash`, which hashes `state.configs` by object + * identity. Sharing therefore depends on one mapping producing one config array, rather than an + * equal array per consumer. + * + * The rules half of that key already holds: `StyleCollection.styles(className)` is keyed by the + * class string, so every element carrying the same class list references one rule set. The config + * half is the one input that is minted per consumer. + */ + +/** A stand-in for the rule set a shared class list resolves to — any object is a valid weak key. */ +const ruleFor = (): WeakKey => ({}); + +/** The smallest `ComponentState` `generateStateHash` reads: it only touches `configs`. */ +const stateFor = (configs: Config[]): ComponentState => { + const observers = new Set(); + const ruleEffect: Effect = { observers, run: () => undefined }; + const inheritedVariables: VariableContextValue = { [VAR_SYMBOL]: true }; + + return { + configs, + inheritedContainers: {}, + inheritedVariables, + ruleEffect, + ruleEffectGetter: (observable) => observable.get(ruleEffect), + styleEffect: { observers, run: () => undefined }, + }; +}; + +const viewMapping = { + className: "style", +} satisfies StyledConfiguration; + +const scrollViewMapping = { + className: "style", + contentContainerClassName: "contentContainerStyle", +} satisfies StyledConfiguration; + +test("two consumers of one mapping share a cache key", () => { + const rules = [ruleFor()]; + + const first = generateStateHash( + stateFor(mappingToConfig(viewMapping)), + rules, + ); + const second = generateStateHash( + stateFor(mappingToConfig(viewMapping)), + rules, + ); + + expect(first).toBe(second); +}); + +test("mappings that differ keep different cache keys", () => { + const rules = [ruleFor()]; + + const view = generateStateHash(stateFor(mappingToConfig(viewMapping)), rules); + const scrollView = generateStateHash( + stateFor(mappingToConfig(scrollViewMapping)), + rules, + ); + + expect(view).not.toBe(scrollView); +}); + +test("a state hash always carries the config, so it is never the empty string", () => { + // The empty string used to double as a no-keys sentinel in `generateStateHash`. With the key a + // join rather than a digest, an empty key list renders as the empty string too — so the sentinel + // and a real state would have shared one cache entry. The config is an unconditional key, which + // is what makes the sentinel unnecessary rather than merely unlikely. + const state = stateFor(mappingToConfig(viewMapping)); + + expect(generateStateHash(state, [])).not.toBe(""); + expect(generateStateHash(state, [ruleFor()])).not.toBe(""); +}); + +test("a mapping that is not an object is refused by name", () => { + // The derivation is cached on the mapping OBJECT. A primitive cannot be a weak-map key, so + // without this guard the failure surfaces as a `WeakMap` error naming nothing the caller wrote. + expect(() => mappingToConfig("style" as never)).toThrow( + /mapping must be an object/u, + ); + expect(() => mappingToConfig(undefined as never)).toThrow( + /mapping must be an object/u, + ); +}); + +test("a mapping mutated after its first use is not re-derived", () => { + // Documented rather than defended: `useCssElement` already froze the derivation per instance, so + // a mutation only ever reached NEWLY mounted elements — the same mapping meaning two things at + // once. Deriving once per mapping settles it on one. + const mapping: Record = { className: "style" }; + const first = mappingToConfig(mapping); + + mapping.className = "contentContainerStyle"; + + expect(mappingToConfig(mapping)).toBe(first); +}); + +test("a mapping built per call still produces an equal config", () => { + // Every wrapper the library ships passes a module constant, but a caller may build the mapping + // inline. That path cannot share on identity and has to keep working unchanged. + expect(mappingToConfig({ className: "style" })).toEqual( + mappingToConfig(viewMapping), + ); +}); diff --git a/src/__tests__/native/family.test.ts b/src/__tests__/native/family.test.ts new file mode 100644 index 00000000..a1022987 --- /dev/null +++ b/src/__tests__/native/family.test.ts @@ -0,0 +1,42 @@ +import { generateHash } from "../../native/react/rules"; +import { family, weakFamily } from "../../native/reactivity"; + +/** + * `family` and `weakFamily` promise one factory call per key, and cached on the result being + * truthy. A factory that legitimately returns `0`, `""` or `false` was therefore re-run on every + * lookup, and its key never settled on a value. + * + * `hashKeyFamily` in `native/react/rules.ts` is such a factory: it hands out `hashKeyCount++`, so + * the first weak key ever hashed is assigned `0` and is the one key that never caches. Its hash + * changes between lookups, which splits every cache keyed on that hash. + */ + +test("weakFamily calls its factory once per key when the result is falsy", () => { + let calls = 0; + const numbers = weakFamily(() => calls++); + const key = {}; + + expect(numbers(key)).toBe(0); + expect(numbers(key)).toBe(0); + expect(calls).toBe(1); +}); + +test("family calls its factory once per key when the result is falsy", () => { + let calls = 0; + const numbers = family(() => calls++); + + expect(numbers("key")).toBe(0); + expect(numbers("key")).toBe(0); + expect(calls).toBe(1); +}); + +test("a weak key hashes to the same value on every lookup", () => { + // The key assigned `0` is the one the falsy-cache miss exposes, and it is whichever key this + // module hashes FIRST. Asserting the value rather than only the agreement is what keeps that + // true: a test added above this one that hashes would take the `0` and leave this passing + // against a key that was never at risk. + const key = {}; + + expect(generateHash([key])).toBe("0"); + expect(generateHash([key])).toBe(generateHash([key])); +}); diff --git a/src/__tests__/native/state-hash.test.ts b/src/__tests__/native/state-hash.test.ts new file mode 100644 index 00000000..abbeffa7 --- /dev/null +++ b/src/__tests__/native/state-hash.test.ts @@ -0,0 +1,73 @@ +import { generateHash } from "../../native/react/rules"; + +/** + * `generateHash` keys the resolved-style cache. Two different sets of rules landing on one key does + * not cost a cache miss — `family` returns the entry it already holds and ignores the rules the + * second caller brought, so that element renders the first element's styles. + * + * Measured before this was injective: a `vertical-align: top` element rendered `object-fit: contain` + * because both rule sets hashed to `18h`. + */ + +/** Distinct weak keys. Any object is a valid one. */ +const keysOf = (count: number): WeakKey[] => + Array.from({ length: count }, () => ({})); + +test("distinct key sets never share a hash", () => { + const keys = keysOf(60); + const owner = new Map(); + + for (const [leftIndex, left] of keys.entries()) { + for (const [offset, right] of keys.slice(leftIndex + 1).entries()) { + const signature = `${String(leftIndex)}+${String(leftIndex + 1 + offset)}`; + const hash = generateHash([left, right]); + const prior = owner.get(hash); + + expect(prior ?? signature).toBe(signature); + owner.set(hash, signature); + } + } + + // Vacuity guard: the loop above must actually have hashed every pair. + expect(owner.size).toBe((keys.length * (keys.length - 1)) / 2); +}); + +test("a key set hashes the same however it is ordered", () => { + // The rule set reaching `generateStateHash` is a Set built in render order, so order-independence + // is what lets two elements with the same rules share one entry at all. + const first: WeakKey = {}; + const second: WeakKey = {}; + const third: WeakKey = {}; + + expect(generateHash([first, second, third])).toBe( + generateHash([third, first, second]), + ); +}); + +test("a key outside WeakKey is refused rather than erased", () => { + // The encoding is exact only if it encodes every member. Skipping one — which the inherited + // `if (!key) continue` did — makes `[a, b]` and `[a, falsy, b]` the same string, and a cache + // key collision hands the second caller the first caller's styles. Unreachable from typed code, + // so this pins the contract rather than a bug: out of domain fails, it does not vanish. + const key: WeakKey = {}; + + expect(() => generateHash([key, undefined as unknown as WeakKey])).toThrow(); +}); + +test("hashing the same key twice answers the same value", () => { + // `hashKeyFamily` assigns each key a number once. A key whose number is not retained hashes + // differently on its second lookup, which splits its cache entry. + const only: WeakKey = {}; + + expect(generateHash([only])).toBe(generateHash([only])); +}); + +test("the encoding is integers, not floats or exponents", () => { + // The ordering runs through a `Float64Array`, so every member is a double by the time it is + // joined. `Number.prototype.toString` renders an integral double without a fraction, and only + // switches to exponential notation past 1e21 — far beyond a key counter. Pinning it here means a + // future change to the array type has to face the question rather than silently reshape the key. + const keys: WeakKey[] = [{}, {}, {}]; + + expect(generateHash(keys)).toMatch(/^\d+(?:,\d+)*$/u); +}); diff --git a/src/__tests__/native/style-cache-bounds.test.tsx b/src/__tests__/native/style-cache-bounds.test.tsx new file mode 100644 index 00000000..d55163e4 --- /dev/null +++ b/src/__tests__/native/style-cache-bounds.test.tsx @@ -0,0 +1,104 @@ +import { View as RNView, type ViewProps } from "react-native"; + +import { render, screen } from "@testing-library/react-native"; +import { copyComponentProperties } from "react-native-css/components/copyComponentProperties"; +import { View } from "react-native-css/components/View"; +import { registerCSS } from "react-native-css/jest"; +import { useCssElement } from "react-native-css/native"; +import type { + StyledConfiguration, + StyledProps, +} from "react-native-css/runtime.types"; + +import { mappingToConfig } from "../../native/react/useNativeCss"; +import { stylesFamily } from "../../native/styles"; + +/** + * Sharing a cache entry across elements is only safe if the entry is (a) released when the last + * element using it unmounts, and (b) not consumed by whichever element reads it first. + * + * Both became load-bearing when the config stopped being per-instance: before that, every element + * held its own entry, so an entry that leaked was one element's worth and an entry that was drained + * was drained only for its owner. + */ + +/** A module constant, exactly as every shipped wrapper declares it. */ +const tintedMapping = { + className: { target: "style", nativeStyleMapping: { color: "tintColor" } }, +} as unknown as StyledConfiguration; + +const Tinted = copyComponentProperties( + RNView, + (props: StyledProps) => + useCssElement(RNView, props, tintedMapping), +); + +test("nothing writes to the config every element now shares", () => { + // While each element minted its own config, a write to one was private. Sharing makes + // "the consumers only read it" load-bearing rather than incidental — and that claim was + // inherited rather than measured. + // + // Checked by VALUE rather than by `Object.freeze`: a write to a frozen object throws only in + // strict mode, and measured here it does not throw at all — so a freeze-based version of this + // test passes whatever the code does, which is worse than not having it. + registerCSS(`.tinted { color: orange; }`); + + const shared = mappingToConfig(tintedMapping); + const before = JSON.stringify(shared); + + render(); + + expect(JSON.stringify(shared)).toBe(before); + expect(screen.getByTestId("unmutated").props.tintColor).toBe("#ffa500"); +}); + +test("a shared entry is not consumed by the first element that reads it", () => { + // `nativeStyleMapping` drains the resolved style in place. That is harmless when every element + // owns its entry and load-bearing once they share one. + registerCSS(`.tinted { color: orange; }`); + stylesFamily.clear(); + + render( + <> + + + + , + ); + + const first = screen.getByTestId("a").props.tintColor; + + expect(first).toBe("#ffa500"); + expect(screen.getByTestId("b").props.tintColor).toBe(first); + expect(screen.getByTestId("c").props.tintColor).toBe(first); +}); + +test("the cache does not grow when a screen is entered and left repeatedly", () => { + // The OOM shape, and the one this PR changes. Entries are not released on unmount — that is a + // separate, pre-existing defect — so what matters is whether a second visit ADDS to them. + // + // With the config minted per instance, a remount produces new config identities, therefore new + // state hashes, therefore new entries, while the old ones stay. Measured on `main`: 2 entries + // after the first visit, 4 after the second, and so on without limit. Deriving the config once + // per mapping makes the second visit reuse the first visit's key, so the count is a function of + // what the app renders rather than of how often it has been rendered. + registerCSS(`.churn-a { color: red; } .churn-b { color: blue; }`); + stylesFamily.clear(); + + const sizes: number[] = []; + + for (let cycle = 0; cycle < 25; cycle += 1) { + const tree = render( + <> + + + , + ); + sizes.push(stylesFamily.size()); + tree.unmount(); + } + + // Every cycle reaches the same count. A ratcheting cache fails on the second entry, not the last. + expect(new Set(sizes).size).toBe(1); + expect(sizes[0]).toBe(2); +}); diff --git a/src/__tests__/native/style-cache-sharing.test.tsx b/src/__tests__/native/style-cache-sharing.test.tsx new file mode 100644 index 00000000..89f0b9bf --- /dev/null +++ b/src/__tests__/native/style-cache-sharing.test.tsx @@ -0,0 +1,43 @@ +import { render } from "@testing-library/react-native"; +import { View } from "react-native-css/components/View"; +import { registerCSS } from "react-native-css/jest"; + +import { stylesFamily } from "../../native/styles"; + +/** + * The end-to-end statement of what the cache is for: it holds one entry per distinct set of + * resolved inputs, not one per mounted element. Everything else in the resolution path is an + * implementation detail of reaching that number. + */ + +const ELEMENTS = 50; + +test("identical elements share one resolved-style cache entry", () => { + registerCSS(`.probe { color: red; width: 10px; }`); + stylesFamily.clear(); + + render( + <> + {Array.from({ length: ELEMENTS }, (_unused, index) => ( + + ))} + , + ); + + expect(stylesFamily.size()).toBe(1); +}); + +test("elements with different class lists keep distinct entries", () => { + // The counter-case: sharing must follow the inputs, not collapse everything onto one entry. + registerCSS(`.red { color: red; } .blue { color: blue; }`); + stylesFamily.clear(); + + render( + <> + + + , + ); + + expect(stylesFamily.size()).toBe(2); +}); diff --git a/src/native/react/rules.ts b/src/native/react/rules.ts index f85a66f9..0b2d8b81 100644 --- a/src/native/react/rules.ts +++ b/src/native/react/rules.ts @@ -289,48 +289,49 @@ const hashKeyFamily = weakFamily(() => hashKeyCount++); export function generateStateHash( state: ComponentState, - iterableKeys?: Iterable, - variables?: WeakKey, - inlineVars?: Set, + iterableKeys: Iterable, ): string { - if (!iterableKeys) { - return ""; - } - - const keys = [state.configs, ...iterableKeys]; - - if (variables) { - keys.push(variables); - } - - if (inlineVars) { - keys.push(...inlineVars); - } - - return generateHash(keys); + // The config is always a key, so this never answers the empty string. That matters: the empty + // string used to double as a no-keys sentinel here, which would have given two different states + // one cache entry now that the key is a join rather than a digest. + return generateHash([state.configs, ...iterableKeys]); } /** - * Quickly generate a unique hash for a set of numbers. - * This is not a cryptographic hash, but it is fast and has a low chance of collision. + * Encode a set of weak keys as a cache key. + * + * This is an exact canonical encoding rather than a hash: it has no chance of collision, and it + * must not be folded back into one. The value keys the resolved-style cache, where two different + * key sets meeting on one string do not cost a cache miss — the second element renders the first + * element's styles. + * + * Every member is encoded. A key skipped here would be erased from the value, so two key sets + * differing only in the skipped member would collide — which is why `WeakKey` is load-bearing + * rather than decorative, and why a value outside it fails at the `WeakMap` rather than passing + * through. */ -const MOD = 9007199254740871; // Largest prime within safe integer range 2^53 -const PRIME = 31; // A smaller prime for mixing export function generateHash(keys: WeakKey[]): string { - let hash = 0; - let product = 1; // Used for mixing to enhance uniqueness + // A Float64Array rather than an array, because `.sort()` on a typed array is numeric and native. + // `Array.prototype.sort` needs a comparator to order numbers, and that comparator is a JS + // function Hermes calls O(n log n) times — measured on a physical device, it is the whole cost of + // ordering here. Float64 rather than Int32 because the key counter is unbounded and Int32 wraps + // silently at 2^31, which would turn two distinct key sets into one string. + const numbers = new Float64Array(keys.length); + let index = 0; for (const key of keys) { - if (!key) continue; // Skip if key is undefined - - const num = hashKeyFamily(key); - hash = (hash ^ num) % MOD; // XOR and modular arithmetic - product = (product * (num + PRIME)) % MOD; // Mix with multiplication + numbers[index] = hashKeyFamily(key); + index += 1; } - // Combine hash and product to form the final hash - hash = (hash + product) % MOD; + // Sorted, so a set of keys encodes the same however it was iterated. The + // caller relies on that: the rule set is a Set built in render order. + // + // Joined rather than folded into a single number, so distinct key sets + // cannot land on one string. This value keys the resolved-style cache, and + // a collision there does not cost a cache miss — it hands one element + // another element's styles. + numbers.sort(); - // Return the hash as a string - return hash.toString(36); + return numbers.join(","); } diff --git a/src/native/react/useNativeCss.ts b/src/native/react/useNativeCss.ts index 11d3ede8..e76c3914 100644 --- a/src/native/react/useNativeCss.ts +++ b/src/native/react/useNativeCss.ts @@ -16,6 +16,7 @@ import { testGuards, type RenderGuard } from "../conditions/guards"; import { cleanupEffect, ContainerContext, + weakFamily, type ContainerContextValue, type Effect, type Getter, @@ -167,9 +168,23 @@ export function useNativeCss( } /** - * Convert the styled() mapping to a config array + * Convert the styled() mapping to a config array. + * + * Derived once per mapping. `generateStateHash` keys the resolved-style cache on `state.configs` + * by object identity, so an equal-but-fresh array per consumer gives each of them its own cache + * entry, its own sorted rules and its own observable. `styled()` already avoids that by deriving + * at module scope; `useCssElement` derives per component instance, and every wrapper this library + * ships passes it a module constant. + * + * Caching on the mapping's identity means a mapping MUTATED after its first use is not re-derived. + * That is a narrowing of behaviour rather than a change of it: `useCssElement` already froze the + * derivation per instance through `useState`, so a live element never saw a mutation either — only + * a newly mounted one did, which made the same mapping mean two things at once. A caller that wants + * a different mapping passes a different object, which is what every call site here already does. */ -export function mappingToConfig(mapping: StyledConfiguration) { +const configForMapping = weakFamily(function ( + mapping: StyledConfiguration, +): Config[] { return Object.entries(mapping).flatMap(([key, value]): Config => { if (value === true) { return { @@ -213,4 +228,24 @@ export function mappingToConfig(mapping: StyledConfiguration) { throw new Error(`styled(): Invalid mapping for ${key}: ${value}`); }); +}); + +/** + * A mapping is a record of prop name to style target, and that is the only shape either entry point + * is typed to accept. Anything else is refused here, by name. + * + * The predicate is deliberately NARROWER than what the `WeakMap` behind `configForMapping` would + * take: a function and an unregistered symbol are both valid weak keys, and both are refused, + * because neither is a mapping. What the guard buys is the message — without it a primitive reaches + * that `WeakMap` and raises `Invalid value used as weak map key` from inside `reactivity`, naming + * neither `styled()` nor the argument that was wrong. + */ +export function mappingToConfig(mapping: StyledConfiguration): Config[] { + if (typeof mapping !== "object" || mapping === null) { + throw new Error( + `styled(): mapping must be an object, received ${mapping === null ? "null" : typeof mapping}`, + ); + } + + return configForMapping(mapping); } diff --git a/src/native/reactivity.ts b/src/native/reactivity.ts index 0824edeb..d2273d17 100644 --- a/src/native/reactivity.ts +++ b/src/native/reactivity.ts @@ -130,7 +130,7 @@ export function family( return Object.assign( (key: Key, args: Args) => { let value = map.get(key); - if (!value) { + if (value === undefined) { value = fn(key, args); map.set(key, value); } @@ -140,6 +140,9 @@ export function family( delete(key: Key) { return map.delete(key); }, + size() { + return map.size; + }, clear() { return map.clear(); }, @@ -167,7 +170,7 @@ export function weakFamily( return Object.assign( (key: Key, args: Args) => { let value = map.get(key); - if (!value) { + if (value === undefined) { value = fn(key, args); map.set(key, value); }