Skip to content

Commit 7bc057a

Browse files
fix(native): one resolved-style cache key per distinct input set
`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 non-object mapping cannot be a weak-map key and is refused by name rather than through a `WeakMap` error. **`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. `generateStateHash`'s empty-string sentinel goes with it: a join makes that value collidable, it was unreachable, and the two parameters no caller passes go at the same time. `family` gains `size()` so the invariant is assertable. Neither it nor `stylesFamily` is reachable from any entry point, so this adds no public API. 50 identical elements: 50 entries before, 1 after. A 150-control screen on a physical Android device: 1785 → 270. 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. 17 tests across five files, each red first.
1 parent f70c402 commit 7bc057a

8 files changed

Lines changed: 417 additions & 36 deletions

File tree

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
import { ScrollView as RNScrollView, View as RNView } from "react-native";
2+
3+
import { generateStateHash } from "../../native/react/rules";
4+
import {
5+
mappingToConfig,
6+
type ComponentState,
7+
type Config,
8+
} from "../../native/react/useNativeCss";
9+
import {
10+
VAR_SYMBOL,
11+
type Effect,
12+
type VariableContextValue,
13+
} from "../../native/reactivity";
14+
import type { StyledConfiguration } from "../../runtime.types";
15+
16+
/**
17+
* The resolved-style cache is keyed by `generateStateHash`, which hashes `state.configs` by object
18+
* identity. Sharing therefore depends on one mapping producing one config array, rather than an
19+
* equal array per consumer.
20+
*
21+
* The rules half of that key already holds: `StyleCollection.styles(className)` is keyed by the
22+
* class string, so every element carrying the same class list references one rule set. The config
23+
* half is the one input that is minted per consumer.
24+
*/
25+
26+
/** A stand-in for the rule set a shared class list resolves to — any object is a valid weak key. */
27+
const ruleFor = (): WeakKey => ({});
28+
29+
/** The smallest `ComponentState` `generateStateHash` reads: it only touches `configs`. */
30+
const stateFor = (configs: Config[]): ComponentState => {
31+
const observers = new Set<Effect>();
32+
const ruleEffect: Effect = { observers, run: () => undefined };
33+
const inheritedVariables: VariableContextValue = { [VAR_SYMBOL]: true };
34+
35+
return {
36+
configs,
37+
inheritedContainers: {},
38+
inheritedVariables,
39+
ruleEffect,
40+
ruleEffectGetter: (observable) => observable.get(ruleEffect),
41+
styleEffect: { observers, run: () => undefined },
42+
};
43+
};
44+
45+
const viewMapping = {
46+
className: "style",
47+
} satisfies StyledConfiguration<typeof RNView>;
48+
49+
const scrollViewMapping = {
50+
className: "style",
51+
contentContainerClassName: "contentContainerStyle",
52+
} satisfies StyledConfiguration<typeof RNScrollView>;
53+
54+
test("two consumers of one mapping share a cache key", () => {
55+
const rules = [ruleFor()];
56+
57+
const first = generateStateHash(
58+
stateFor(mappingToConfig(viewMapping)),
59+
rules,
60+
);
61+
const second = generateStateHash(
62+
stateFor(mappingToConfig(viewMapping)),
63+
rules,
64+
);
65+
66+
expect(first).toBe(second);
67+
});
68+
69+
test("mappings that differ keep different cache keys", () => {
70+
const rules = [ruleFor()];
71+
72+
const view = generateStateHash(stateFor(mappingToConfig(viewMapping)), rules);
73+
const scrollView = generateStateHash(
74+
stateFor(mappingToConfig(scrollViewMapping)),
75+
rules,
76+
);
77+
78+
expect(view).not.toBe(scrollView);
79+
});
80+
81+
test("a state hash always carries the config, so it is never the empty string", () => {
82+
// The empty string used to double as a no-keys sentinel in `generateStateHash`. With the key a
83+
// join rather than a digest, an empty key list renders as the empty string too — so the sentinel
84+
// and a real state would have shared one cache entry. The config is an unconditional key, which
85+
// is what makes the sentinel unnecessary rather than merely unlikely.
86+
const state = stateFor(mappingToConfig(viewMapping));
87+
88+
expect(generateStateHash(state, [])).not.toBe("");
89+
expect(generateStateHash(state, [ruleFor()])).not.toBe("");
90+
});
91+
92+
test("a mapping that is not an object is refused by name", () => {
93+
// The derivation is cached on the mapping OBJECT. A primitive cannot be a weak-map key, so
94+
// without this guard the failure surfaces as a `WeakMap` error naming nothing the caller wrote.
95+
expect(() => mappingToConfig("style" as never)).toThrow(
96+
/mapping must be an object/u,
97+
);
98+
expect(() => mappingToConfig(undefined as never)).toThrow(
99+
/mapping must be an object/u,
100+
);
101+
});
102+
103+
test("a mapping mutated after its first use is not re-derived", () => {
104+
// Documented rather than defended: `useCssElement` already froze the derivation per instance, so
105+
// a mutation only ever reached NEWLY mounted elements — the same mapping meaning two things at
106+
// once. Deriving once per mapping settles it on one.
107+
const mapping: Record<string, string> = { className: "style" };
108+
const first = mappingToConfig(mapping);
109+
110+
mapping.className = "contentContainerStyle";
111+
112+
expect(mappingToConfig(mapping)).toBe(first);
113+
});
114+
115+
test("a mapping built per call still produces an equal config", () => {
116+
// Every wrapper the library ships passes a module constant, but a caller may build the mapping
117+
// inline. That path cannot share on identity and has to keep working unchanged.
118+
expect(mappingToConfig({ className: "style" })).toEqual(
119+
mappingToConfig(viewMapping),
120+
);
121+
});
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
import { generateHash } from "../../native/react/rules";
2+
import { family, weakFamily } from "../../native/reactivity";
3+
4+
/**
5+
* `family` and `weakFamily` promise one factory call per key, and cached on the result being
6+
* truthy. A factory that legitimately returns `0`, `""` or `false` was therefore re-run on every
7+
* lookup, and its key never settled on a value.
8+
*
9+
* `hashKeyFamily` in `native/react/rules.ts` is such a factory: it hands out `hashKeyCount++`, so
10+
* the first weak key ever hashed is assigned `0` and is the one key that never caches. Its hash
11+
* changes between lookups, which splits every cache keyed on that hash.
12+
*/
13+
14+
test("weakFamily calls its factory once per key when the result is falsy", () => {
15+
let calls = 0;
16+
const numbers = weakFamily<object, number>(() => calls++);
17+
const key = {};
18+
19+
expect(numbers(key)).toBe(0);
20+
expect(numbers(key)).toBe(0);
21+
expect(calls).toBe(1);
22+
});
23+
24+
test("family calls its factory once per key when the result is falsy", () => {
25+
let calls = 0;
26+
const numbers = family<string, number>(() => calls++);
27+
28+
expect(numbers("key")).toBe(0);
29+
expect(numbers("key")).toBe(0);
30+
expect(calls).toBe(1);
31+
});
32+
33+
test("a weak key hashes to the same value on every lookup", () => {
34+
// The first key this module hashes is the one assigned `0`, so it is the one that exposes the
35+
// miss. Nothing else in this file hashes, which is what keeps it first.
36+
const key = {};
37+
38+
expect(generateHash([key])).toBe(generateHash([key]));
39+
});
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
import { generateHash } from "../../native/react/rules";
2+
3+
/**
4+
* `generateHash` keys the resolved-style cache. Two different sets of rules landing on one key does
5+
* not cost a cache miss — `family` returns the entry it already holds and ignores the rules the
6+
* second caller brought, so that element renders the first element's styles.
7+
*
8+
* Measured before this was injective: a `vertical-align: top` element rendered `object-fit: contain`
9+
* because both rule sets hashed to `18h`.
10+
*/
11+
12+
/** Distinct weak keys. Any object is a valid one. */
13+
const keysOf = (count: number): WeakKey[] =>
14+
Array.from({ length: count }, () => ({}));
15+
16+
test("distinct key sets never share a hash", () => {
17+
const keys = keysOf(60);
18+
const owner = new Map<string, string>();
19+
20+
for (const [leftIndex, left] of keys.entries()) {
21+
for (const [offset, right] of keys.slice(leftIndex + 1).entries()) {
22+
const signature = `${String(leftIndex)}+${String(leftIndex + 1 + offset)}`;
23+
const hash = generateHash([left, right]);
24+
const prior = owner.get(hash);
25+
26+
expect(prior ?? signature).toBe(signature);
27+
owner.set(hash, signature);
28+
}
29+
}
30+
31+
// Vacuity guard: the loop above must actually have hashed every pair.
32+
expect(owner.size).toBe((keys.length * (keys.length - 1)) / 2);
33+
});
34+
35+
test("a key set hashes the same however it is ordered", () => {
36+
// The rule set reaching `generateStateHash` is a Set built in render order, so order-independence
37+
// is what lets two elements with the same rules share one entry at all.
38+
const first: WeakKey = {};
39+
const second: WeakKey = {};
40+
const third: WeakKey = {};
41+
42+
expect(generateHash([first, second, third])).toBe(
43+
generateHash([third, first, second]),
44+
);
45+
});
46+
47+
test("hashing the same key twice answers the same value", () => {
48+
// `hashKeyFamily` assigns each key a number once. A key whose number is not retained hashes
49+
// differently on its second lookup, which splits its cache entry.
50+
const only: WeakKey = {};
51+
52+
expect(generateHash([only])).toBe(generateHash([only]));
53+
});
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
import { View as RNView, type ViewProps } from "react-native";
2+
3+
import { render, screen } from "@testing-library/react-native";
4+
import { copyComponentProperties } from "react-native-css/components/copyComponentProperties";
5+
import { View } from "react-native-css/components/View";
6+
import { registerCSS } from "react-native-css/jest";
7+
import { useCssElement } from "react-native-css/native";
8+
import type {
9+
StyledConfiguration,
10+
StyledProps,
11+
} from "react-native-css/runtime.types";
12+
13+
import { mappingToConfig } from "../../native/react/useNativeCss";
14+
import { stylesFamily } from "../../native/styles";
15+
16+
/**
17+
* Sharing a cache entry across elements is only safe if the entry is (a) released when the last
18+
* element using it unmounts, and (b) not consumed by whichever element reads it first.
19+
*
20+
* Both became load-bearing when the config stopped being per-instance: before that, every element
21+
* held its own entry, so an entry that leaked was one element's worth and an entry that was drained
22+
* was drained only for its owner.
23+
*/
24+
25+
/** A module constant, exactly as every shipped wrapper declares it. */
26+
const tintedMapping = {
27+
className: { target: "style", nativeStyleMapping: { color: "tintColor" } },
28+
} as unknown as StyledConfiguration<typeof RNView>;
29+
30+
const Tinted = copyComponentProperties(
31+
RNView,
32+
(props: StyledProps<ViewProps, typeof tintedMapping>) =>
33+
useCssElement(RNView, props, tintedMapping),
34+
);
35+
36+
test("nothing writes to the config every element now shares", () => {
37+
// While each element minted its own config, a write to one was private. Sharing makes
38+
// "the consumers only read it" load-bearing rather than incidental — and that claim was
39+
// inherited rather than measured.
40+
//
41+
// Checked by VALUE rather than by `Object.freeze`: a write to a frozen object throws only in
42+
// strict mode, and measured here it does not throw at all — so a freeze-based version of this
43+
// test passes whatever the code does, which is worse than not having it.
44+
registerCSS(`.tinted { color: orange; }`);
45+
46+
const shared = mappingToConfig(tintedMapping);
47+
const before = JSON.stringify(shared);
48+
49+
render(<Tinted className="tinted" testID="unmutated" />);
50+
51+
expect(JSON.stringify(shared)).toBe(before);
52+
expect(screen.getByTestId("unmutated").props.tintColor).toBe("#ffa500");
53+
});
54+
55+
test("a shared entry is not consumed by the first element that reads it", () => {
56+
// `nativeStyleMapping` drains the resolved style in place. That is harmless when every element
57+
// owns its entry and load-bearing once they share one.
58+
registerCSS(`.tinted { color: orange; }`);
59+
stylesFamily.clear();
60+
61+
render(
62+
<>
63+
<Tinted className="tinted" testID="a" />
64+
<Tinted className="tinted" testID="b" />
65+
<Tinted className="tinted" testID="c" />
66+
</>,
67+
);
68+
69+
const first = screen.getByTestId("a").props.tintColor;
70+
71+
expect(first).toBe("#ffa500");
72+
expect(screen.getByTestId("b").props.tintColor).toBe(first);
73+
expect(screen.getByTestId("c").props.tintColor).toBe(first);
74+
});
75+
76+
test("the cache does not grow when a screen is entered and left repeatedly", () => {
77+
// The OOM shape, and the one this PR changes. Entries are not released on unmount — that is a
78+
// separate, pre-existing defect — so what matters is whether a second visit ADDS to them.
79+
//
80+
// With the config minted per instance, a remount produces new config identities, therefore new
81+
// state hashes, therefore new entries, while the old ones stay. Measured on `main`: 2 entries
82+
// after the first visit, 4 after the second, and so on without limit. Deriving the config once
83+
// per mapping makes the second visit reuse the first visit's key, so the count is a function of
84+
// what the app renders rather than of how often it has been rendered.
85+
registerCSS(`.churn-a { color: red; } .churn-b { color: blue; }`);
86+
stylesFamily.clear();
87+
88+
const sizes: number[] = [];
89+
90+
for (let cycle = 0; cycle < 25; cycle += 1) {
91+
const tree = render(
92+
<>
93+
<View className="churn-a" />
94+
<View className="churn-b" />
95+
</>,
96+
);
97+
sizes.push(stylesFamily.size());
98+
tree.unmount();
99+
}
100+
101+
// Every cycle reaches the same count. A ratcheting cache fails on the second entry, not the last.
102+
expect(new Set(sizes).size).toBe(1);
103+
expect(sizes[0]).toBe(2);
104+
});
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import { render } from "@testing-library/react-native";
2+
import { View } from "react-native-css/components/View";
3+
import { registerCSS } from "react-native-css/jest";
4+
5+
import { stylesFamily } from "../../native/styles";
6+
7+
/**
8+
* The end-to-end statement of what the cache is for: it holds one entry per distinct set of
9+
* resolved inputs, not one per mounted element. Everything else in the resolution path is an
10+
* implementation detail of reaching that number.
11+
*/
12+
13+
const ELEMENTS = 50;
14+
15+
test("identical elements share one resolved-style cache entry", () => {
16+
registerCSS(`.probe { color: red; width: 10px; }`);
17+
stylesFamily.clear();
18+
19+
render(
20+
<>
21+
{Array.from({ length: ELEMENTS }, (_unused, index) => (
22+
<View key={index} className="probe" />
23+
))}
24+
</>,
25+
);
26+
27+
expect(stylesFamily.size()).toBe(1);
28+
});
29+
30+
test("elements with different class lists keep distinct entries", () => {
31+
// The counter-case: sharing must follow the inputs, not collapse everything onto one entry.
32+
registerCSS(`.red { color: red; } .blue { color: blue; }`);
33+
stylesFamily.clear();
34+
35+
render(
36+
<>
37+
<View className="red" />
38+
<View className="blue" />
39+
</>,
40+
);
41+
42+
expect(stylesFamily.size()).toBe(2);
43+
});

0 commit comments

Comments
 (0)