Cache TextPainter plain text value to improve performance

[tgucio]

This PR improves performance in text fields and selectable text by avoiding redundant calls to InlineSpan.toPlainText() which can be expensive with multiple spans:

• cache TextPainter plain text
• use this cached value in Editable
• expose the cached value for EditableText and TextSelection widgets

The performance gain is expected in: caret painting (and e.g. movement with keyboard) and selection painting (updating selection rects while dragging).

Text-exempt: optimisation only without change in behaviour.

Pre-launch Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
• I signed the [CLA].
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is [test-exempt].
• All existing and new tests are passing.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[tgucio]

/cc @justinmc @LongCatIsLooong @Renzo-Olivares

[whiskeyPeak]

Any numbers on how much of an improvement this change is producing?

[tgucio]

@whiskeyPeak I didn't run detailed benchmarking but with a larger text in multiple spans and when moving the caret using arrow keys, repeated calls to toPlainText() were taking loads of CPU time and contributing to a visible lag (the 1st contributor were the text input channel calls). Note the text painting itself won't be affected by the change - it's what's painted on top of text (caret and selection). Updated the PR description to mention this.

[justinmc]

Just to make sure we think this through: this PR is assuming that the cost of recalculating the plain text is greater than the cost of maintaining the cached string in memory. Do you have any evidence that that is true, and are there any cases we should keep in mind where it's not true? (very large strings, deeply nested spans, etc.)

Overall my gut says this would be an improvement. Are you able to add tests? It might not be possible to test the caching, but you could definitely test that the plainText getters on RenderEditable and TextPainter give the correct value.

[tgucio]

@justinmc The CPU cycle cost is negligible with typical form fields but for larger texts with different spans, that can quickly become a long series of children-visiting calls. Today every caret move or selection drag gesture will repeat this.

Now for the memory cost... quick search took me to this huffpost article:

According to Amazon’s great Text Stats feature, the median length for all books is about 64,000 words. The figure was found through looking at a number of books’ text stats, until "Brave New World"‘s 64,531 word count landed in the exact center of all books–50% of books have fewer words and 50% of books have more words.

So with the average English word length at 4.7 characters, with UTF-16 strings it'd be just shy of 600kB to cache the entire Brave New World.

I'll add a test in RenderEditable and TextPainter to read the cached value on first paint and on subsequent InlineSpan updates.

[tgucio]

@justinmc changes done, PTAL

[justinmc]

LGTM 👍 with a nit. Also it looks like the PR needs to be update with master, can you do that? My fault for taking so long to re-review.

I'm on board with the performance argument here. Thanks for spelling it out in your argument above.

CC @LongCatIsLooong for secondary review.

[flutter-dashboard]

This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled.

[LongCatIsLooong]

I think it will be better if TextSpan could do the caching, inline span trees are immutable. But there could be some lints that currently prevent an instance of an @immutable class from caching things or lazily compute stored properties, and if you take the @immutable off you probably won't be allowed to implement equality. @goderbauer do you think we can disable the lint that only allows @immutable classes to override ==?

[tgucio]

@LongCatIsLooong InlineSpan, TextSpan etc. are all immutables with const constructors so this wouldn't be a minor change and might be quite against the initial design. Then there's the avoid_equals_and_hash_code_on_mutable_classes lint.
I guess another approach would be to calculate plain text in the constructor using static methods in all *Span classes. In any case that's probably a discussion for another PR - this one is quick & pretty cheap.

[LongCatIsLooong]

Yeah the lints don't seem to be that big of a problem since they're just artificially imposed but the const constructor is likely a real limitation. My concern is TextPainter.plainText exposes implementation details (the placeholders for example), and exposing that as a public getter could lead to misuse of the API. Looking at the references to TextPainter.plainText outside of the file:

• Everything that looks like plainText.length doesn't have to actually build the string since it only wants the length. It's probably something InlineSpan should provide a getter for.
• plainText.codeUnitAt should be text.codeUnitAt.
• final String text = renderEditable.plainText; in editable_text.dart is just for cache invalidation and there are better ways to do that (InlineSpan.compare)
• _getStart/EndGlyphHeight I think this one shouldn't use plainText either

[tgucio]

@LongCatIsLooong can you think of any specific ways to abuse the plainText getter from TextPainter? In the end it's just a String getter for something that can be read from the text InlineSpan tree anyway and it accurately represents what the TextPainter is currently working with for other API calls. But perhaps we can add more comments if needed.
To me the additional advantage of cleaning this up and having a single place to store the plain text (in TextPainter) is that it avoids using InlineSpan.toPlainText with different includeSemanticsLabels value in different places.