Use a separate `TextPainter` for intrinsics calculation in `RenderEditable` and `RenderParagraph` #144577

LongCatIsLooong · 2024-03-05T00:20:36Z

Use a dedicated TextPainter for intrinsic size calculation in RenderEditable and RenderParagraph.

This is an implementation detail so the change should be covered by existing tests. Performance wise this shouldn't be significantly slower since SkParagraph caches the result of slower operations across different paragraphs. Existing benchmarks should be able to catch potential regressions (??).

The reason for making this change is to make sure that intrinsic size computations don't destroy text layout artifacts, so I can expose the text layout as a stream of immutable TextLayout objects, to signify other render objects that text-layout-dependent-cache (such as caches for getBoxesForRange which can be relatively slow to compute) should be invalidated and markNeedsPaint needs to be called if the painting logic depended on text layout.
Without this change, the intrinsics/dry layout calculations will add additional events to the text layout stream, which violates the "dry"/non-destructive contract.

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…raph`

…er-for-intrinsics

LongCatIsLooong · 2024-03-11T22:23:04Z

@goderbauer I'm not 100% sure this is covered by existing benchmarks. If not what kind of benchmarks do you think would be helpful? One with Text in an IntrinsicWidth/Height and keeps resizing?

goderbauer · 2024-03-12T16:42:21Z

packages/flutter/lib/src/rendering/editable.dart

-  void markNeedsTextLayout() {
-    _textLayoutLastMaxWidth = null;
-    _textLayoutLastMinWidth = null;
+  void _markNeedsTextLayout() {


Does this still need to be a separate method? Could we just call markNeedsLayout directly instead of _markNeedsTextLayout?

goderbauer · 2024-03-12T16:46:25Z

packages/flutter/lib/src/rendering/editable.dart

@@ -868,7 +875,8 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin,
      return;
    }
    _textPainter.strutStyle = value;
-    markNeedsTextLayout();
+    _textIntrinsicsCache?.strutStyle = value;


Slightly anoying that we have to remember setting everything twice. I wonder, would it be an option to just copy the text painter configuration over when we attempt to do any intrinsics calculations?

goderbauer · 2024-03-12T16:50:04Z

I'm not 100% sure this is covered by existing benchmarks.

As far as I know we don't really have a benchmark covering these cases.

One with Text in an IntrinsicWidth/Height and keeps resizing?

That seems like a good idea to cover this change.

For #144577. There's no promise that the performance will be great when `IntrinsicHeight/IntrinsicWidth` is used extensively but it's not that uncommon of a widget.

…er-for-intrinsics

…)" (#145029) Reverts: #145007 Initiated by: LongCatIsLooong Reason for reverting: https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_android%20microbenchmarks/15561/overview Original PR Author: LongCatIsLooong Reviewed By: {goderbauer} This change reverts the following previous change: For #144577. There's no promise that the performance will be great when `IntrinsicHeight/IntrinsicWidth` is used extensively but it's not that uncommon of a widget.

LongCatIsLooong · 2024-03-13T16:45:06Z

I ran the newly added microbenchmark locally and didn't observe a performance regression. The Google testing failures are scuba changes

goderbauer

LGTM

goderbauer · 2024-03-15T17:52:59Z

packages/flutter/lib/src/rendering/editable.dart

+      ..strutStyle = _textPainter.strutStyle
+      ..textWidthBasis = _textPainter.textWidthBasis
+      ..textHeightBehavior = _textPainter.textHeightBehavior;
+  }


I wonder if we could somehow have a test to verify that the two text painters remain in sync? I don't have any ideas of how to do that, though.

I was hoping the dry size check in RenderBox.size setter would help catch inconsistencies in the two text painters?

goderbauer · 2024-03-15T17:55:39Z

packages/flutter/test/rendering/paragraph_test.dart

+    paragraph.paint(MockPaintingContext(), Offset.zero);
+    expect(getRectForA(), const Rect.fromLTWH(0, 0, 10, 10));
+  });
+


Do we have enough existing tests to cover that when you change the configuration of a RenderParagraph or RenderEditable that the new configuration is properly accounted for in the intrinsics calculations?

Added a few tests. I couldn't think of ways to test some configurations using the test font (textDirection for example).

goderbauer · 2024-03-15T17:56:19Z

The Google testing failures are scuba changes

Wait, why does this change affect any goldens? Isn't that kinda unexpected?

…er-for-intrinsics

LongCatIsLooong · 2024-03-15T20:57:57Z

The Google testing failures are scuba changes

Wait, why does this change affect any goldens? Isn't that kinda unexpected?

TextPainter sometimes tries to lay out the text using the text's maxIntrinsic width instead of the real input width, and I think that could cause the pixel shifts in the scubas.

…enderEditable` and `RenderParagraph` (flutter/flutter#144577)

flutter/flutter@71606af...7c860dd 2024-03-17 [email protected] Roll Flutter Engine from cfbf11d757d1 to 4c420dd2dfd9 (2 revisions) (flutter/flutter#145280) 2024-03-17 [email protected] Factor out use of "print" in flutter_goldens (flutter/flutter#144846) 2024-03-16 [email protected] Roll Flutter Engine from dd4050334ff3 to cfbf11d757d1 (1 revision) (flutter/flutter#145275) 2024-03-16 [email protected] Roll Flutter Engine from 618ee6c8ba34 to dd4050334ff3 (1 revision) (flutter/flutter#145270) 2024-03-16 [email protected] Roll Flutter Engine from e4602aa149d8 to 618ee6c8ba34 (1 revision) (flutter/flutter#145266) 2024-03-16 [email protected] Roll Flutter Engine from 91b9cf24d328 to e4602aa149d8 (1 revision) (flutter/flutter#145259) 2024-03-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland #128236 "Improve build output for all platforms" (#143166)" (flutter/flutter#145261) 2024-03-16 [email protected] Reland #128236 "Improve build output for all platforms" (flutter/flutter#143166) 2024-03-16 [email protected] Introduce `Split` curve (flutter/flutter#143130) 2024-03-16 [email protected] Roll Flutter Engine from 120e52b8a1c2 to 91b9cf24d328 (1 revision) (flutter/flutter#145256) 2024-03-16 [email protected] Roll Flutter Engine from 3abc25139959 to 120e52b8a1c2 (1 revision) (flutter/flutter#145254) 2024-03-16 [email protected] Roll Flutter Engine from 6f180615e6ac to 3abc25139959 (1 revision) (flutter/flutter#145253) 2024-03-16 [email protected] Platform view devicelab ad banner scroll list real ads (flutter/flutter#145239) 2024-03-16 [email protected] Roll Flutter Engine from 6ddaa40bf1a6 to 6f180615e6ac (1 revision) (flutter/flutter#145251) 2024-03-16 [email protected] Roll Flutter Engine from d46c1fa17dda to 6ddaa40bf1a6 (1 revision) (flutter/flutter#145250) 2024-03-16 [email protected] Roll Flutter Engine from 7f15dc1200bc to d46c1fa17dda (2 revisions) (flutter/flutter#145248) 2024-03-16 [email protected] Use a separate `TextPainter` for intrinsics calculation in `RenderEditable` and `RenderParagraph` (flutter/flutter#144577) 2024-03-16 [email protected] Roll Flutter Engine from 2db915c64e6e to 7f15dc1200bc (1 revision) (flutter/flutter#145245) 2024-03-15 [email protected] Roll Flutter Engine from ba1115c30381 to 2db915c64e6e (2 revisions) (flutter/flutter#145240) 2024-03-15 [email protected] Roll Flutter Engine from c2fd5333a5d2 to ba1115c30381 (3 revisions) (flutter/flutter#145237) 2024-03-15 [email protected] Renaming the virtual device Nexus5 name to Nexus5.gce_x86 (flutter/flutter#145225) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

TahaTesser · 2024-03-20T11:10:58Z

@LongCatIsLooong
My PR #143932 fails web HTML renderer tests after this PR and potentially also failing #142804 tests.

LongCatIsLooong · 2024-03-20T18:04:46Z

@LongCatIsLooong My PR #143932 fails web HTML renderer tests after this PR and potentially also failing #142804 tests.

I'll take a look in a bit.

…enderEditable` and `RenderParagraph` (flutter/flutter#144577)

flutter/flutter@71606af...7c860dd 2024-03-17 [email protected] Roll Flutter Engine from cfbf11d757d1 to 4c420dd2dfd9 (2 revisions) (flutter/flutter#145280) 2024-03-17 [email protected] Factor out use of "print" in flutter_goldens (flutter/flutter#144846) 2024-03-16 [email protected] Roll Flutter Engine from dd4050334ff3 to cfbf11d757d1 (1 revision) (flutter/flutter#145275) 2024-03-16 [email protected] Roll Flutter Engine from 618ee6c8ba34 to dd4050334ff3 (1 revision) (flutter/flutter#145270) 2024-03-16 [email protected] Roll Flutter Engine from e4602aa149d8 to 618ee6c8ba34 (1 revision) (flutter/flutter#145266) 2024-03-16 [email protected] Roll Flutter Engine from 91b9cf24d328 to e4602aa149d8 (1 revision) (flutter/flutter#145259) 2024-03-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland #128236 "Improve build output for all platforms" (#143166)" (flutter/flutter#145261) 2024-03-16 [email protected] Reland #128236 "Improve build output for all platforms" (flutter/flutter#143166) 2024-03-16 [email protected] Introduce `Split` curve (flutter/flutter#143130) 2024-03-16 [email protected] Roll Flutter Engine from 120e52b8a1c2 to 91b9cf24d328 (1 revision) (flutter/flutter#145256) 2024-03-16 [email protected] Roll Flutter Engine from 3abc25139959 to 120e52b8a1c2 (1 revision) (flutter/flutter#145254) 2024-03-16 [email protected] Roll Flutter Engine from 6f180615e6ac to 3abc25139959 (1 revision) (flutter/flutter#145253) 2024-03-16 [email protected] Platform view devicelab ad banner scroll list real ads (flutter/flutter#145239) 2024-03-16 [email protected] Roll Flutter Engine from 6ddaa40bf1a6 to 6f180615e6ac (1 revision) (flutter/flutter#145251) 2024-03-16 [email protected] Roll Flutter Engine from d46c1fa17dda to 6ddaa40bf1a6 (1 revision) (flutter/flutter#145250) 2024-03-16 [email protected] Roll Flutter Engine from 7f15dc1200bc to d46c1fa17dda (2 revisions) (flutter/flutter#145248) 2024-03-16 [email protected] Use a separate `TextPainter` for intrinsics calculation in `RenderEditable` and `RenderParagraph` (flutter/flutter#144577) 2024-03-16 [email protected] Roll Flutter Engine from 2db915c64e6e to 7f15dc1200bc (1 revision) (flutter/flutter#145245) 2024-03-15 [email protected] Roll Flutter Engine from ba1115c30381 to 2db915c64e6e (2 revisions) (flutter/flutter#145240) 2024-03-15 [email protected] Roll Flutter Engine from c2fd5333a5d2 to ba1115c30381 (3 revisions) (flutter/flutter#145237) 2024-03-15 [email protected] Renaming the virtual device Nexus5 name to Nexus5.gce_x86 (flutter/flutter#145225) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

LongCatIsLooong added 2 commits February 29, 2024 15:23

Use a separate TextPainter for intrinsics calculate in `RenderParag…

9ba4596

…raph`

wip

dbf258d

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 5, 2024

LongCatIsLooong changed the title ~~Separate text painter for intrinsics~~ Use a separate TextPainter for intrinsics calculation in RenderEditable and RenderParagraph Mar 5, 2024

add _computeTextMetricsIfNeeded

b746a7d

LongCatIsLooong force-pushed the separate-text-painter-for-intrinsics branch from 86d179b to b746a7d Compare March 5, 2024 02:29

Merge remote-tracking branch 'upstream/main' into separate-text-paint…

40e3bb8

…er-for-intrinsics

LongCatIsLooong requested a review from goderbauer March 11, 2024 22:16

goderbauer reviewed Mar 12, 2024

View reviewed changes

LongCatIsLooong mentioned this pull request Mar 12, 2024

Add a microbenchmark for text intrinsic height layout #145007

Merged

9 tasks

review

2979374

LongCatIsLooong force-pushed the separate-text-painter-for-intrinsics branch from afcd70d to 2979374 Compare March 12, 2024 21:00

Merge remote-tracking branch 'upstream/main' into separate-text-paint…

9285a05

…er-for-intrinsics

auto-submit bot mentioned this pull request Mar 12, 2024

Reverts "Add a microbenchmark for text intrinsic height layout (#145007)" #145029

Merged

9 tasks

LongCatIsLooong mentioned this pull request Mar 15, 2024

Expose listenable text layout from RenderEditable / RenderParagraph #145190

Closed

9 tasks

goderbauer approved these changes Mar 15, 2024

View reviewed changes

LongCatIsLooong added 2 commits March 15, 2024 11:08

Merge remote-tracking branch 'upstream/main' into separate-text-paint…

13d1891

…er-for-intrinsics

add tests

d795196

skip strut tests on HTML renderer

3ad8461

LongCatIsLooong force-pushed the separate-text-painter-for-intrinsics branch from e369503 to 3ad8461 Compare March 15, 2024 21:53

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 16, 2024

auto-submit bot merged commit cc01701 into flutter:master Mar 16, 2024
67 checks passed

LongCatIsLooong deleted the separate-text-painter-for-intrinsics branch March 16, 2024 01:16

engine-flutter-autoroll mentioned this pull request Mar 16, 2024

Roll Flutter from c01d7f069861 to e7baebdce72d (85 revisions) flutter/packages#6343

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2024

cc01701 Use a separate TextPainter for intrinsics calculation in `R…

481583a

…enderEditable` and `RenderParagraph` (flutter/flutter#144577)

engine-flutter-autoroll mentioned this pull request Mar 16, 2024

Roll Flutter from c01d7f069861 to 16d0731363c3 (86 revisions) flutter/packages#6344

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2024

cc01701 Use a separate TextPainter for intrinsics calculation in `R…

225b8e2

…enderEditable` and `RenderParagraph` (flutter/flutter#144577)

engine-flutter-autoroll mentioned this pull request Mar 17, 2024

Roll Flutter from 71606af8d82e to 7c860ddf9705 (21 revisions) flutter/packages#6345

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 17, 2024

cc01701 Use a separate TextPainter for intrinsics calculation in `R…

cea2210

…enderEditable` and `RenderParagraph` (flutter/flutter#144577)

TahaTesser mentioned this pull request Mar 20, 2024

Fix MenuItemButton overflow #143932

Merged

9 tasks

TahaTesser mentioned this pull request Mar 21, 2024

[HTML renderer] size computed in performLayout is different than computeDryLayout in some cases #145527

Open

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024

cc01701 Use a separate TextPainter for intrinsics calculation in `R…

6621979

…enderEditable` and `RenderParagraph` (flutter/flutter#144577)

zmtzawqlp mentioned this pull request May 15, 2024

didn't reset _textIntrinsicsCache #144577 #148386

Closed

zmtzawqlp added a commit to fluttercandies/extended_text that referenced this pull request May 31, 2024

Merge flutter/flutter#144577

f71d24c

zmtzawqlp added a commit to fluttercandies/extended_text that referenced this pull request Aug 8, 2024

Merge flutter/flutter#144577

3ad6fe8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a separate `TextPainter` for intrinsics calculation in `RenderEditable` and `RenderParagraph` #144577

Use a separate `TextPainter` for intrinsics calculation in `RenderEditable` and `RenderParagraph` #144577

LongCatIsLooong commented Mar 5, 2024 •

edited

Loading

LongCatIsLooong commented Mar 11, 2024

goderbauer Mar 12, 2024

goderbauer Mar 12, 2024

goderbauer commented Mar 12, 2024

LongCatIsLooong commented Mar 13, 2024

goderbauer left a comment

goderbauer Mar 15, 2024

LongCatIsLooong Mar 15, 2024

goderbauer Mar 15, 2024

LongCatIsLooong Mar 15, 2024

goderbauer commented Mar 15, 2024 •

edited

Loading

LongCatIsLooong commented Mar 15, 2024

TahaTesser commented Mar 20, 2024

LongCatIsLooong commented Mar 20, 2024

Use a separate TextPainter for intrinsics calculation in RenderEditable and RenderParagraph #144577

Use a separate TextPainter for intrinsics calculation in RenderEditable and RenderParagraph #144577

Conversation

LongCatIsLooong commented Mar 5, 2024 • edited Loading

Pre-launch Checklist

LongCatIsLooong commented Mar 11, 2024

goderbauer Mar 12, 2024

Choose a reason for hiding this comment

goderbauer Mar 12, 2024

Choose a reason for hiding this comment

goderbauer commented Mar 12, 2024

LongCatIsLooong commented Mar 13, 2024

goderbauer left a comment

Choose a reason for hiding this comment

goderbauer Mar 15, 2024

Choose a reason for hiding this comment

LongCatIsLooong Mar 15, 2024

Choose a reason for hiding this comment

goderbauer Mar 15, 2024

Choose a reason for hiding this comment

LongCatIsLooong Mar 15, 2024

Choose a reason for hiding this comment

goderbauer commented Mar 15, 2024 • edited Loading

LongCatIsLooong commented Mar 15, 2024

TahaTesser commented Mar 20, 2024

LongCatIsLooong commented Mar 20, 2024

Use a separate `TextPainter` for intrinsics calculation in `RenderEditable` and `RenderParagraph` #144577

Use a separate `TextPainter` for intrinsics calculation in `RenderEditable` and `RenderParagraph` #144577

LongCatIsLooong commented Mar 5, 2024 •

edited

Loading

goderbauer commented Mar 15, 2024 •

edited

Loading