Skip to content

Avoid copying text fragments and re-attributing runs when building NSAttributedString - #59154

Open
hryhoriiK97 wants to merge 1 commit into
react:mainfrom
hryhoriiK97:ios-attributed-string-trims
Open

hryhoriiK97 wants to merge 1 commit into
react:mainfrom
hryhoriiK97:ios-attributed-string-trims

Conversation

@hryhoriiK97

@hryhoriiK97 hryhoriiK97 commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary:

RCTNSAttributedStringFromAttributedString (ReactCommon/react/renderer/textlayoutmanager/platform/ios/.../RCTAttributedTextUtils.mm) converts a paragraph's AttributedString into the NSAttributedString TextKit lays out and draws. It runs once per paragraph per measurement when the attributed-string cache misses, so for a screen of distinct labels it runs once per label.

Current behavior. Two pieces of redundant work per fragment:

  • the loop iterates getFragments() by value (for (auto fragment : ...)), copying each Fragment: its std::string, its TextAttributes, and a ShadowView with three shared_ptr members (props, event emitter, state), so three atomic retain/release pairs per fragment;
  • the event emitter is attached in a second addAttributes:range: pass over the fragment's string right after it was created with its attributes, which splits and re-merges the attribute runs TextKit just built.

With this change. Fragments are iterated by reference, and a text fragment receives the event emitter in the initial attribute dictionary passed to initWithString:attributes:. Attachment fragments are untouched: their string comes from attributedStringWithAttachment:, and they keep the existing addAttributes:range: pass. On main that pass applied to both kinds whenever the fragment's parent has a component handle, so the attributes on every run, text and attachment alike, are the same as before.

Changelog:

[IOS] [CHANGED] - Build a paragraph's NSAttributedString without copying each text fragment or re-attributing its runs

Test Plan:

  • node scripts/clang-format.js --check on the changed file passes.
  • RNTester built with xcodebuild in Release for a physical iPhone 15 Pro; the Text examples render as before.
  • Screenshots of a 1000-label list and of a set of edge cases (right-aligned text with an inline view, centered multi-line text, empty text with an explicit size, truncation across nested colours, RTL with an inline view, clipped fixed height, two-line tail ellipsis) are byte-identical before and after.
  • Measurement. RNTester Playground mounting 1000 unique single-line and wrapping labels in one commit; a timer around the conversion call inside -[RCTTextLayoutManager measureAttributedString:...], summed per transaction, and Fabric's transaction telemetry for the layout total. Two builds of the same tree differing only in this file, with the font cache (Cache resolved custom fonts on iOS Fabric #59153) present in both so that font resolution does not dominate the conversion. The button presses were driven by Argent on the physical iPhone, so both builds ran the identical sequence (5 mounts and unmounts per font). iPhone 15 Pro, Release, medians of 4 to 5 mounts:
1000 labels before this PR change
conversion, system font 8.0 ms 7.0 ms −13%
conversion, fontFamily: 'Avenir' 7.7 ms 6.4 ms −17%

A second run of the same comparison gave 6.4 ms and 6.8 ms for the two cases, so the run-to-run spread is about 0.5 ms and the saving is 12 to 20% of the conversion step. The layout total moves within noise (47.6 → 46.9 ms system, 35.9 → 34.6 ms Avenir); the saving is confined to the conversion step, as expected.

🤖 Generated with Claude Code

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 16, 2026
@facebook-github-tools facebook-github-tools Bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Oct 16, 2026

@cipolleschi cipolleschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is doing a lot of changes that are unrelated from the PR title/purpose.
Also, most of them are changes that are not really needed: they are small refactoring that only adds complexity and take time to review.

Please do small focused PRs that do a single thing and do not add not-relevant changes to the PR

Comment on lines +423 to +427
[[NSMutableAttributedString attributedStringWithAttachment:attachment] mutableCopy];
if (eventEmitterWrapper != nil) {
[nsAttributedStringFragment addAttribute:RCTAttributedStringEventEmitterKey
value:eventEmitterWrapper
range:NSMakeRange(0, nsAttributedStringFragment.length)];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this changes the behavior of React Native and has nothing to do with the PR title.

Before we were not attaching the event emitter if the fragment was an attachment, now we are.

If you want this change, please create a separate PR with the proper title and summary. Let's have small and focused PRs, please.

@hryhoriiK97 hryhoriiK97 Oct 17, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

I believe this one is not a behavior change: on main, the same addAttributes call already set the emitter on attachment fragments too (RCTNSAttributedStringFragmentWithAttributesFromFragment applied it to both kinds). To make that clearer, I've reworked the diff so the attachment code stays untouched - only text fragments now receive the emitter in their initial attributes. Happy to adjust if you'd prefer a different shape 🙂

@hryhoriiK97
hryhoriiK97 force-pushed the ios-attributed-string-trims branch from 1343122 to 3fee4c7 Compare October 17, 2026 11:34
@hryhoriiK97 hryhoriiK97 changed the title Trim NSAttributedString construction for text on iOS Avoid copying text fragments and re-attributing runs when building NSAttributedString Oct 17, 2026
@hryhoriiK97
hryhoriiK97 force-pushed the ios-attributed-string-trims branch from 3fee4c7 to dc8e2cc Compare October 17, 2026 11:45
…AttributedString

`RCTNSAttributedStringFromAttributedString` iterated the fragments by value,
copying each one (its string, its text attributes and a shadow view with
three shared pointers), and attached the event emitter to a text fragment in
a second `addAttributes:range:` pass over the string it had just created with
its attributes, which splits and re-merges the attribute runs. Iterate by
reference and put the emitter into a text fragment's initial attribute
dictionary. Attachment fragments keep the existing second pass, so the
attributes on every run are unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@hryhoriiK97
hryhoriiK97 force-pushed the ios-attributed-string-trims branch from dc8e2cc to 37b84be Compare October 17, 2026 11:47
@hryhoriiK97

Copy link
Copy Markdown
Contributor Author

This PR is doing a lot of changes that are unrelated from the PR title/purpose. Also, most of them are changes that are not really needed: they are small refactoring that only adds complexity and take time to review.

Please do small focused PRs that do a single thing and do not add not-relevant changes to the PR

@cipolleschi sorry about this! I've updated the PR so it does a single thing: it only avoids copying each text fragment and re-attributing the runs when building the NSAttributedString. The unrelated refactors are removed, and the title, description and measurements are updated to match the smaller diff. Thanks for the review!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants