Fix extreme TextInput slowness on Android

[gnprice]

This reverts 5898817 "Implement letterSpacing on Android >= 5.0".
Testing shows that that commit is the cause of #19126, where in a
controlled TextInput after some text is first added, then deleted,
further interaction with the TextInput becomes extremely slow.

Fixes #19126.

Test Plan

Tried the repro case from #19126 without this change, then with it.
The issue reproduces, then doesn't.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[pull-bot]

	Warnings
⚠️	📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

[gnprice]

/cc @motiz88 as the author of #17398, which became the commit that caused #19126 and which this PR would revert. If the code can be fixed to keep that feature while fixing this issue, that would of course be better than merging this revert :-)

I'd like to CC the reviewer of #17398 too, but AFAICT from the PR thread, the review happened internally within Facebook and I don't see any other details.

[motiz88]

Huh, weird! I wasn't aware of #19126 until now, thanks @gnprice.

/cc @janicduplessis @hramos

One thing I'd personally love to see is if we can preserve the <Text> behaviour and only regress <TextInput>.

Secondarily, it would be nice to figure out why this is slow at all, and maybe extract an Android platform bug report out of this (though now that I've said this, I've shifted the probability towards a glaring memory leak in my own code 😅)

I'll have some time this weekend to investigate. I don't know what the release plans are exactly, but if the maintainers feel this patch should go out as-is, I don't have a problem with that.

[hramos]

#17398 was merged by 5898817 which was reviewed internally by @mdvacca

[janicduplessis]

I agree it would be nice to only revert the text input part, I think it should work if we just revert the code in ReactEditText.java and ReactTextInputManager.java.

[hramos]

Can we revert just the TextInput part?

[shafy]

Any updates on in which release this PR will be included? #18916 is also the same issue.

[kelset]

@shafy depending on the author responsiveness, we'll surely try to cherry pick this for 0.56.0 since its release is still a couple weeks (more or less) away

[stueynet]

@gnprice Any chance you can respond to @hramos change request? This would allow it to be merged to master and cherry picked for 56 - react-native-community/releases#14

[hramos]

Feel free to send a PR that reverts just the Text Input changes, if the feedback here is not addressed.

[gnprice]

Hi all, and thanks @janicduplessis and @hramos for the feedback!

Today I spent some more time experimenting with this. I tried this suggestion:

I think it should work if we just revert the code in ReactEditText.java and ReactTextInputManager.java.

Unfortunately it doesn't work :-/ -- the issue still repros.

If on top of that I revert this one additional hunk:

       }
+      if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
+        if (textShadowNode.mLetterSpacing != Float.NaN) {
+          ops.add(new SetSpanOperation(
+            start,
+            end,
+            new CustomLetterSpacingSpan(textShadowNode.mLetterSpacing)));
+        }
+      }
       if (textShadowNode.mFontSize != UNSET) {

then that fixes the issue! That leaves part of the original changes from #17398 in place -- there's a setLetterSpacing method, and it's controlled by a letterSpacing prop and also called from setAllowFontScaling; and there's a CustomLetterSpacingSpan class. But as far as I understand how the original commit works, this hunk is at the crux of it, and without it the rest of the changes don't have any effect.

So you can see exactly what I tried, here's my branch:
gnprice/react-native@v0.55.4...text-experiments-0.55

So it looks like this is going to take some debugging of the actual logic, if we want to fix the issue without just reverting the feature completely.

[gnprice]

... Oho. I bet this is the problem:

        if (textShadowNode.mLetterSpacing != Float.NaN) {

Because NaN is special, that will always be true -- even if mLetterSpacing is also Float.NaN, which is its default value.

That means every text shadow node will create a CustomLetterSpacingSpan around its contents, even when the letterSpacing prop was never set. It must be that we're somehow ending up with large numbers of these shadow nodes -- that sounds like a bug in itself, but I guess it's normally low-impact -- and this condition is failing to prune them from causing a bunch of work here.

The correct way to write this condition is with Float.isNaN. I'll go see if that fixes the issue.

[gnprice]

New version pushed! This one is a simple one-line fix, just fixing the specific bug in #17398 without reverting anything:

       if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
-        if (textShadowNode.mLetterSpacing != Float.NaN) {
+        if (!Float.isNaN(textShadowNode.mLetterSpacing)) {
           ops.add(new SetSpanOperation(

@hramos , @janicduplessis , please take a look!

[hramos]

I tried to approve and merge this, but it looks like you haven't signed the CLA yet, @gnprice :)

[gnprice]

CLA signed! :) Hopefully the bot will chime in confirming that shortly.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[motiz88]

@gnprice Argh, I should have known better than to introduce a NaN comparison bug! Well spotted & thanks for being on top of this.

[shafy]

@motiz88 happens to the best =P

[gnprice]

Ouch, the commit message in the final commit is wrong because it's taken from my PR message (which describes the original version, a revert of that previous commit) rather than from my actual commit (which describes what it actually does).

I guess as a former (happy!) Phabricator user, I should have known that was how it was going to work. :-P Next time I'll be sure to edit the PR message, just like when using Phabricator.

Thanks @hramos and @janicduplessis !

[Noitidart]

@gnprice I don't get why if (!Float.isNaN(textShadowNode.mLetterSpacing)) { is a fix. Because if we do set letterSpacing then this will wrap everything in a span no? So this only fixes if letterSpacing is not set no?

Maybe we should remove the letterSpacing support in this case?

[gnprice]

@Noitidart

if we do set letterSpacing then this will wrap everything in a span no? So this only fixes if letterSpacing is not set no?

That's right. If you look at the surrounding code, this follows the pattern of about 8 other features:

      if (textShadowNode.mFontSize != UNSET) {
        ops.add(new SetSpanOperation(start, end, new AbsoluteSizeSpan(textShadowNode.mFontSize)));
      }
// ...
      if (textShadowNode.mIsUnderlineTextDecorationSet) {
        ops.add(new SetSpanOperation(start, end, new UnderlineSpan()));
      }
      if (textShadowNode.mIsLineThroughTextDecorationSet) {
        ops.add(new SetSpanOperation(start, end, new StrikethroughSpan()));
      }

In each case: if some prop is set, then it's implemented by creating a span. This makes sense to me architecturally, if the implementation works out the way I imagine the authors of this class intended: one formatting prop on one React component, or one spot where the formatting needs to change inside a React component, produces one Android text span (or possibly an O(1) handful for some features.) That corresponds well with my understanding of how the Android text span API is designed to be used.

It's still a bit of a puzzle to me why we're going through this code so many times that getting this condition wrong was such a problem. As I wrote above, it seems like that means we're ending up with tons and tons of instances of this ReactBaseTextShadowNode class (well, of its subclasses), just from typing a bunch of text into a single boring TextInput. That isn't how this class reads to me like it's designed to be used.

Whatever's causing that, though, I think it's unrelated to the letterSpacing feature, which looks like it's doing the same thing here (after this bugfix) as a bunch of other formatting features.

Take a look at #20119, which I just filed today; I think that issue is related to this puzzle.

[Noitidart]

Thanks @gnprice for that detail!

[jenni-divvito]

Is there a way to get a version of this patch onto 0.55 short of us just forking RN and applying it?

[hramos]

You can check out the 0.55-stable branch and cherry-pick this fix onto your local checkout, and build a release from that.