Repository navigation
Read only the drawn child's mix-blend-mode in Android drawChild - #59016
Closed
YevheniiKotyrlo wants to merge 1 commit into
Closed
YevheniiKotyrlo wants to merge 1 commit into
YevheniiKotyrlo wants to merge 1 commit into
Conversation
ReactViewGroup.drawChild and ReactRootView.drawChild asked BlendModeHelper.needsIsolatedLayer(this) before reading the drawn child's mix_blend_mode tag. needsIsolatedLayer reads that tag on every child of the group, and dispatchDraw calls drawChild once per child, so recording a group of N children read N^2 tags. With no blend mode set anywhere the scan never short-circuits, so the whole cost lands on views that never use the feature. A child carrying the tag makes needsIsolatedLayer true by definition, and a child without it draws with no layer either way, so the group scan adds nothing to the decision drawChild makes. drawChild now reads the child's own tag; draw() keeps its group-wide check, which runs once per recording.
meta-cla
Bot
added
the
CLA Signed
facebook-github-tools
Bot
added
the
Shared with Meta
Contributor
|
/review |
🤖 AI code reviewDecision: Ready for human review Overall PR risk: Low. This change modifies existing behavior in Android drawChild blend-mode compositing in ReactViewGroup and ReactRootView. Blast radius is limited to Android Q+ views that use mixBlendMode, with direct rollback and no data or API change. No critical or warning findings remain; the most plausible breakage is incorrect isolated-layer compositing if the per-child tag read diverged from the prior group check. No findings. This review is advisory — it never blocks a merge and never auto-approves. |
|
@javache has imported this pull request. If you are a Meta employee, you can view this in D124965507. |
meta-codesync Bot
pushed a commit
that referenced
this pull request
Oct 16, 2026
Summary: Two workflows stopped working after the move to `react/react-native`, because their regexes only allow `github.com/facebook/react-native`: - **Merged pull requests no longer get the release FAQ comment.** Before the move, `react-native-bot` posted "This pull request was successfully merged by author in **sha**" with links to "When will my fix make it into a release?" and "How to file a pick request?". The last one is on #56769 (June 9). Pull requests merged on October 15 (#59122, #59050, #59016) only have the `meta-codesync` comment. - **Issues that link a pull request as the reproducer still get `Needs: Repro`.** Examples: #58970, #58495, #58445, #55708. The bot's message for `Needs: Repro` asks for a pull request that edits `RNTesterPlayground.js`, but the bot does not accept a link to one. ### Why 1. `.github/workflows/close-pr.yml` looks for `Pull Request resolved: https://github.com/facebook/react-native/pull/N` in the commit message. Commits now say `https://github.com/react/react-native/pull/N`, so `getClosedPrIfExists` returns `undefined` and the script stops before `createComment`. #57167 changed the `if:` line in this file to `react/react-native`, but not the regex. The regex is written with escaped slashes (`facebook\/react-native`). 2. `.github/workflow-scripts/checkForReproducer.js` builds the pull request pattern in a template literal with single backslashes. In a template literal, `\d` is just `d`, so the pattern is `https?://github.com/facebook/react-native/pull/d+/?`. It did not match real links even before the move (the line was added in #50848). The other two patterns in the same function use `\\` and work. In the whole repo, `facebook\/react-native` appears only in these two lines. ### Fix Both regexes now accept `facebook` and `react`. People still write links with `facebook`: the four issues above all do. In `checkForReproducer.js`, the pattern now uses `\\` like the other two patterns. `(?:...)` does not capture, so the group numbers in `close-pr.yml` do not change. The new test file is `checkForReproducer-links-test.js`, so it does not conflict with `checkForReproducer-test.js` from #58429. That PR fixes a different problem in the same script: the bot treats its own `Needs: Repro` label as a maintainer change. Because of that, an issue that already has the label keeps it after an edit, also with this PR. New issues with a pull request link do not get the label. ## Changelog: [INTERNAL] [FIXED] - Fix pull request link regexes in the close-pr and check-for-reproducer workflows after the move to react/react-native Pull Request resolved: #59128 Test Plan: Windows 11, Node 22.17.1, Yarn 1.22.22. New Jest test for `checkForReproducer` with 4 cases: a link to a pull request in `react/react-native`, the same link with `facebook/react-native`, a pull request in another repository, and no link. ``` yarn test .github/workflow-scripts ``` | Code | Result | | --- | --- | | `main` (e04aaf6) | 2 of the 4 new tests fail: both pull request links are not accepted | | this PR | 11 suites, 72 tests pass | Each part of the fix is needed: - `facebook|react` with single backslashes: both link tests fail. - Double backslashes with only `facebook`: the `react` link test fails. `close-pr.yml` runs an inline script, so it has no Jest test. I took `prClosingRegex` from the file and ran `getClosedPrIfExists` (copied from the file) on real commit messages from `main`: | Commit message | `main` | this PR | | --- | --- | --- | | #59016: `Pull Request resolved: https://github.com/react/react-native/pull/59016` | `undefined` | `59016` | | #56769: `Pull Request resolved: https://github.com/facebook/react-native/pull/56769` | `56769` | `56769` | | "Update React Native DevTools binaries", no pull request link | `undefined` | `undefined` | `npx prettier --check` passes for the 3 files. Not covered: - I did not run the workflows. They run on push to `main` and on issue events in this repository. - `yarn lint` does not check these files, because ESLint skips the `.github` folder. Reviewed By: cipolleschi Differential Revision: D125650549 Pulled By: cortinico fbshipit-source-id: c1d1b49381730f755d13e3c822ac1df33a4808b8
meta-codesync Bot
pushed a commit
that referenced
this pull request
Oct 20, 2026
…59063) Summary: When Yoga lays out a container again, it clones every child whose layout it touches, including unchanged siblings. A `ParagraphShadowNode` cloned that way (no new props, no new children) drops its memoized `content_` (the attributed string) and `measuredLayouts_`. **Current behavior.** In `layout()` the clone rebuilds its `AttributedString` and compares it with the one in its `ParagraphState`. After the paragraph's first commit the comparison fails even though nothing changed. `Fragment::operator==` compares `parentShadowView.layoutMetrics`, and the string stored in state captured those metrics before the paragraph had been laid out. So `updateStateIfNeeded` creates a new state for every sibling paragraph: - On Android each sibling then gets an `UPDATE_STATE` mount instruction. The UI thread rebuilds its Spannable, calls `setText`, lays the text out again and redraws it. - A trace of "mount 1000 labels, then change one" shows 1000 `ReactTextViewManager.updateState`, 1000 `setText` and 1000 `onDraw` calls for the one change. - With `enablePreparedTextLayout`, the clone also has no measured layout to reuse, so every sibling is laid out again on the JS thread. **With this change.** A clone whose fragment has neither props nor children keeps `content_` and `measuredLayouts_`. `getContent` drops both if the font-size multiplier or layout direction they were built for no longer matches. Unchanged siblings then produce no state update and no re-layout. A kept layout is only reused at the pixel density it was prepared for. After a density change the paragraph prepares a new layout, so the state refresh from #59049 (`fixTextLayoutOnPixelDensityChange`) still applies with `enablePreparedTextLayout`. Content with attachments (inline views inside the text) is not kept: the attachments point at the children the content was built from, and `layout()` replaces those children with laid-out clones, so in a later revision they would point at nodes only the previous revision owns. **Why.** Any update inside a container with many text siblings (non-virtualized lists, settings screens, forms, tables) currently costs UI-thread work for every sibling, and with prepared text layout also a JS-thread layout for every sibling. ## Changelog: [GENERAL] [FIXED] - Avoid state updates and re-layout of unchanged `Text` siblings when a sibling changes Pull Request resolved: #59063 Test Plan: - `./gradlew :packages:rn-tester:android:app:assembleDebug -PreactNativeArchitectures=arm64-v8a` and `yarn format-check-cpp` pass on this branch. - Fantom could not be built locally: the bundled `nlohmann/json.hpp` fails with this machine's Apple clang. I'm relying on CI for it. - Screenshots were pixel-identical before and after a sequence of updates (all text, one item, colour, font size, container width), in four configurations with and without `enablePreparedTextLayout`. - iOS shares this code path but was not built or measured. - The pixel-density check (last commit) was compiled against the Android build's flags with `-fsyntax-only` but not run; no density change was tested on a device. - Galaxy A16 release build, cold start and 15 flings through two long feeds whose text contains inline views, 16 runs per variant: the first version of this PR crashed in 4 (SIGSEGV in `ParagraphShadowNode::layout`, reading an attachment the previous revision had released), the current version in none. **Benchmarks.** RNTester release build with a text benchmark screen that is not part of this PR. Medians, main vs. this PR, variants interleaved. Emulator (Pixel 10 Pro AVD, API 37), current `main` vs. this branch, 6 runs: | scenario | metric | main | this PR | change | |---|---|--:|--:|--:| | 1000 labels, change one | UI-thread CPU | 21.8 ms | 5.2 ms | −76% | | | time to pixels | 58.6 ms | 33.6 ms | −43% | | 300 strings, change one | UI-thread CPU | 29.0 ms | 6.0 ms | −79% | | | time to pixels | 49.3 ms | 32.5 ms | −34% | | 1000 labels, change one, `enablePreparedTextLayout` | JS-thread CPU | 30.4 ms | 18.5 ms | −39% | | | time to pixels | 66.2 ms | 49.1 ms | −26% | | mounts | | | | no significant change | Physical Samsung Galaxy A16, the same change on an earlier `main` (`d4d0813969c`), 8 runs. #59016 was applied in both arms. | scenario | main | this change | change | |---|--:|--:|--:| | 1000 labels, change one right after mount: time to pixels | 222 ms | 78 ms | −65% | | 300 strings, change one right after mount: time to pixels | 164 ms | 36 ms | −78% | | 1000 labels, change one, prepared text: time to pixels | 228 ms | 73 ms | −68% | | same: JS-thread CPU | 132 ms | 44 ms | −67% | 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: cipolleschi Differential Revision: D126131800 Pulled By: javache fbshipit-source-id: 88d934a7ecb188430c19a6c62135f4602da7b99f
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
2 participants
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.
Summary:
ReactViewGroup.drawChildandReactRootView.drawChildcallBlendModeHelper.needsIsolatedLayer(this)before they read the drawn child'smix_blend_modetag.needsIsolatedLayerreads that tag on every child of the group, anddispatchDrawcallsdrawChildonce per child, so recording the display list of a group with N children reads N² tags. When no child sets a blend mode — the common case —anynever short-circuits, so the whole scan runs for every child of every group that records.I found it profiling flings over a grid of a few hundred views on two release builds: sampling the main thread,
needsIsolatedLayerunderdrawChildwas 37% of display-list recording on a Snapdragon 845 (Android 10) and 46% on a Snapdragon 8 Gen 2 (Android 14).The guard adds nothing to the decision
drawChildmakes. A child that carries the tag makesneedsIsolatedLayertrue by definition, and a child without it draws with no layer whatever the guard says. So both overrides now read the drawn child's own tag.draw()keeps its group-wide check, which decides whether the whole group renders offscreen and runs once per recording.I searched the tracker for
needsIsolatedLayer,BlendModeHelper,mix_blend_modeanddrawChildand found no existing report or PR.Changelog:
[ANDROID] [FIXED] - Drawing a view group no longer reads every sibling's mix-blend-mode for each child it draws
Test Plan:
./gradlew :packages:react-native:ReactAndroid:testDebugUnitTest --tests "com.facebook.react.views.view.ReactViewGroupTest" -Preact.internal.useHermesStable=true: onmainthe newdrawing a child reads no sibling's blend modetest fails withexpected:<[3]> but was:<[21]>— each child of a 20-child group has its tag read 21 times, against 3 in a 2-child group; with the fix both groups read each child equally often, and all 19 tests in the class pass.a child with a blend mode draws into a layer with that blend modetest records[null, MULTIPLY]— the group's isolated layer, then the child's — before and after the change; with the child's layer skipped it fails withExpecting actual: [null] to contain exactly (and in same order): [null, MULTIPLY]../gradlew :packages:react-native:ReactAndroid:testDebugUnitTest -PenableWarningsAsErrors=true -Preact.internal.useHermesStable=true: 707 tests, 0 failures.node ./scripts/format-kotlin.js --checkandnode ./scripts/format-java.js --checkpass.needsIsolatedLayerran 1,608 times for 1,400drawChildcalls before and 182 times for 3,234 after, every one of them fromdraw(). AmixBlendMode: 'multiply'view in the same app still blends.build_android).