Permalink
Browse files

Fix extreme TextInput slowness on Android (#19645)

Summary:
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.

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

Differential Revision: D8675230

Pulled By: hramos

fbshipit-source-id: e2c2d352ee781898721b2dff4738572d1a6b7471
  • Loading branch information...
gnprice authored and facebook-github-bot committed Jun 28, 2018
1 parent b3ef1c3 commit 5017b86b525e3ef6023f0f8a88e6fd1cf98024e0
@@ -119,7 +119,7 @@ private static void buildSpannedFromShadowNode(
start, end, new BackgroundColorSpan(textShadowNode.mBackgroundColor)));
}
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
if (textShadowNode.mLetterSpacing != Float.NaN) {
if (!Float.isNaN(textShadowNode.mLetterSpacing)) {
ops.add(new SetSpanOperation(
start,
end,

2 comments on commit 5017b86

@hramos

This comment has been minimized.

Show comment
Hide comment
@hramos

hramos Jun 28, 2018

Contributor

The commit description here is out of date because it's taken from the original PR description. The actual commit that implemented this change has the following text:

Fix extreme TextInput slowness on Android

Because NaN is special, the `!=` version of this condition will
always be true -- even if `mLetterSpacing` is also `Float.NaN`, which
is its default value.  It should instead be `!Float.isNaN(...)`.

The effect of the broken condition is that every text shadow node will
create a `CustomLetterSpacingSpan` around its contents, even when the
`letterSpacing` prop was never set.

Empirically, that in turn causes #19126: in a controlled TextInput
after some text is first added, then deleted, further interaction
with the TextInput becomes extremely slow.  Perhaps 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); then this
code would have caused them each to generate more work to do.  In
any case, fixing this condition causes that issue to disappear.

The affected logic was introduced between v0.54 and v0.55, in #17398
aka 5898817 "Implement letterSpacing on Android >= 5.0".

Fixes #19126.
Contributor

hramos replied Jun 28, 2018

The commit description here is out of date because it's taken from the original PR description. The actual commit that implemented this change has the following text:

Fix extreme TextInput slowness on Android

Because NaN is special, the `!=` version of this condition will
always be true -- even if `mLetterSpacing` is also `Float.NaN`, which
is its default value.  It should instead be `!Float.isNaN(...)`.

The effect of the broken condition is that every text shadow node will
create a `CustomLetterSpacingSpan` around its contents, even when the
`letterSpacing` prop was never set.

Empirically, that in turn causes #19126: in a controlled TextInput
after some text is first added, then deleted, further interaction
with the TextInput becomes extremely slow.  Perhaps 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); then this
code would have caused them each to generate more work to do.  In
any case, fixing this condition causes that issue to disappear.

The affected logic was introduced between v0.54 and v0.55, in #17398
aka 5898817 "Implement letterSpacing on Android >= 5.0".

Fixes #19126.
@gnprice

This comment has been minimized.

Show comment
Hide comment
@gnprice

gnprice Jun 28, 2018

Contributor

Ah, thanks, @hramos !

Next time I'll be sure to update the PR description so the commit that's merged gets the right message. :-)

Contributor

gnprice replied Jun 28, 2018

Ah, thanks, @hramos !

Next time I'll be sure to update the PR description so the commit that's merged gets the right message. :-)

Please sign in to comment.