perf(mobile): cache initial SkPath in usePathTransition#777
Draft
AndreiCalazans wants to merge 1 commit into
Draft
perf(mobile): cache initial SkPath in usePathTransition#777AndreiCalazans wants to merge 1 commit into
AndreiCalazans wants to merge 1 commit into
Conversation
`usePathTransition` was calling `Skia.Path.MakeFromSVGString` on every render to compute `initialSkiaPath`, but the result is only ever consumed as the initial value of the four `useSharedValue` calls below it. Since `useSharedValue` ignores its argument on every render after the first, every later parse was discarded. Switch to a `useState` lazy initializer so the parse runs exactly once per hook instance. In an Android prod CPU trace (1:03 capture), this hot path accounted for ~64% of all `MakeFromSVGString` self-time (~350ms over the trace) across `AnimatedPath` and `DottedArea` chart renders. `useState` is used rather than `useMemo` because `useMemo` is documented as a perf hint that React may discard, and any realistic dependency array would either fail the exhaustive-deps lint rule (`[]`) or re-parse on every `currentPath` change while the shared-value initializers continue to ignore the new result.
Collaborator
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
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.
What changed? Why?
usePathTransition(packages/mobile/src/visualizations/chart/utils/transition.ts) was callingSkia.Path.MakeFromSVGString(initialPath ?? currentPath)directly in the function body on every render. The result is only consumed as the initial value of fouruseSharedValuecalls immediately below it. BecauseuseSharedValueignores its argument on every render after the first, every later parse was discarded.This PR moves the parse into a
useStatelazy initializer so it runs exactly once per hook instance.Why
useState(notuseMemo)useMemois documented as a performance hint that React may discard.useState's lazy initializer is the only documented "called exactly once per component instance" primitive.useMemodependency array ([initialPath, currentPath]) would re-parse on everycurrentPathchange while the shared-value initializers continued to ignore the new result — re-introducing the bug in a slightly different form.useMemodependency array ([]) would lint-fail underreact-hooks/exhaustive-deps, and is precisely the anti-pattern that "useuseStatefor one-time init" docs warn against.Impact
In an Android prod CPU profile (1:03 capture, Hermes) on a chart-heavy screen:
AnimatedPath > usePathTransition(render body)DottedArea > usePathTransition(render body)Path > useDerivedValue > initialUpdaterRun(static branch)commitHookEffectListMount(data-change useEffect)MakeFromSVGStringThe render-body bucket (rows 1 and 2) is the one this PR eliminates: ~31 calls / ~351 ms, ≈64% of all
MakeFromSVGStringself-time. After the fix, only 1 of those calls per hook instance remains (the legitimate first-render parse).Root cause (required for bugfixes)
N/A — perf-only refactor; no behaviour change.
UI changes
No UI change.
useSharedValuealready ignores all subsequent values, so the visible animation is identical — only the parse cost is removed.Testing
How has it been tested?
packages/mobile/src/visualizations/chart/utils/__tests__/transition.test.tspasses; fullnx test mobilesuite (170 suites / 1845 tests) passes.nx lint mobileclean ontransition.ts.nx typecheck mobileclean.Testing instructions
Render any chart that uses
Path/AnimatedPath/DottedArea(e.g.LineChart,AreaChart, sparkline) and confirm that:initialPath(orcurrentPathwhen noinitialPathis provided).interpolatePathexactly as before.transitions.update: nullstill produces an instant transition.Optional perf verification: capture a Hermes CPU profile while scrolling a chart-heavy screen and confirm
[HostFunction] MakeFromSVGStringself-time is reduced and that the render-body bucket disappears from the sandwich view ofusePathTransition.Change management
type=routine
risk=low
impact=sev5
automerge=false