Dispose Paragraph objects #110627

dnfield · 2022-08-30T22:26:37Z

Addresses most of #110601, except one case that's a bit difficult right now - the FlutterLogo decoration does not have a lifecycle and creates a text painter.

Once this is done, we can remove the bogus accounting we do for paragraph object size in dart:ui.

This patch also includes one stray vertices.dispose, since I added that API in the same engine patch as the one for Paragraph and they're both now available.

dnfield · 2022-08-30T23:23:26Z

Will need flutter/engine#35815

Also needs something else for HTML - I'll need to take more time to figure out if we're using a disposed object becuase we're disposing it too early or if it's because we should stop trying to use it.

dnfield · 2022-08-31T21:14:32Z

Will also need flutter/engine#35835

yjbanov · 2022-09-01T04:17:47Z

packages/flutter/test/painting/text_painter_test.dart

  }, skip: isBrowser && !isCanvasKit); // https://github.com/flutter/flutter/issues/56308
+
+  test('TextPainter - debugDisposed', () {
+    final TextPainter painter = TextPainter();


Check that at this point debugDispose is false?

yjbanov · 2022-09-01T04:21:11Z

packages/flutter/lib/src/painting/text_painter.dart

+
+  /// Releases the resources associated with this painter.
+  ///
+  /// After disposal, this painter is unusable.


nit: I think this sentence does not need a comma.

yjbanov · 2022-09-01T04:22:31Z

packages/flutter/lib/src/painting/text_painter.dart

+  ///
+  /// Only for use when asserts are enabled.
+  bool get debugDisposed {
+    bool disposed = false;


Maybe throw instead of returning false in non-debug modes?

Done. I think we should come up with a unified strategy for this and put it in the style guide. I'm somewhat inclined to think throwing is too aggressive, but we can hash that out separately from this PR.

yjbanov · 2022-09-01T04:23:04Z

packages/flutter/lib/src/painting/text_painter.dart

+
+  /// Whether this object has been disposed or not.
+  ///
+  /// Only for use when asserts are enabled.


I think we typically refer to it as "debug mode", since on the command-line the option is --debug.

Some customers disable asserts in debug mode.

yjbanov · 2022-09-01T04:25:42Z

packages/flutter/lib/src/cupertino/date_picker.dart

-
-    return painter.maxIntrinsicWidth;
+    try {
+      return painter.maxIntrinsicWidth;


Using try blocks for control flow is kind of an anti-pattern. Consider storing the return value in a variable, then returning the variable.

We want to make sure to dispose in case an exception gets thrown by the getter.

yjbanov · 2022-09-01T04:26:54Z

packages/flutter/test/widgets/text_test.dart

@@ -1070,6 +1070,7 @@ void main() {
    final ui.Paragraph paragraph = builder.build();
    paragraph.layout(const ui.ParagraphConstraints(width: 1000));
    expect(paragraph.getBoxesForRange(2, 2), isEmpty);
+    paragraph.dispose();


WidgetTester performs checks for dangling app state, such as mouse pointers. Should we also have it proactively look for leaked paragraph?

Or perhaps the leak detector that @polina-c is working on can help here?

We'd need changes in dart:ui to support this.

Polina's work should help here. There's another option where we make the engine itself aware of this kind of thing and throw an exception when it happens, but that's possibly a heavy hammer and prone to flakes.

flutter-dashboard · 2022-09-01T04:48:30Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

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

Changes reported for pull request #110627 at sha f0d4cfe

flutter-dashboard · 2022-09-03T01:40:25Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

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

Changes reported for pull request #110627 at sha 7e2fe2a

LongCatIsLooong

The text part lgtm (modulo comments from other reviewers). But for my own edification, are all NativeFieldWrapperClasses need a dispose method now or it's case-by-case?

Also there are 153 references to the TextPainter constructor in g3. By definition this is not a breaking change but to me this feels quite breaking (if developers don't update they're going to start leaking TextPainters?).

LongCatIsLooong · 2022-09-06T07:50:18Z

packages/flutter/lib/src/widgets/banner.dart

@@ -144,8 +153,8 @@ class BannerPainter extends CustomPainter {
      ..drawRect(_kRect, _paintShadow)
      ..drawRect(_kRect, _paintBanner);
    const double width = _kOffset * 2.0;
-    _textPainter.layout(minWidth: width, maxWidth: width);
-    _textPainter.paint(canvas, _kRect.topLeft + Offset(0.0, (_kRect.height - _textPainter.height) / 2.0));
+    _textPainter!.layout(minWidth: width, maxWidth: width);


nit: nothing resets _parepared to false so _textPainter is a lazily initialized final var. Maybe change _textPainter back to final late so we can remove the exclamation marks here?

Developers are already leaking text painters today, they just have no way of disposing them. We should send out an announcement though and I can look into doing a g3 CL.

After dispose, the object should not be used - I don't think resetting prepared makes sense there, but we could add asserts about making sure methods aren't called after disposal.

And having a non-nullable but disposable object is often a little tricky, it's better to be able to null it out to prevent usage after disposal.

It seems paragraphs can get different disposal signals:

Leak: the developer doesn't call dispose, so a live paragraph is GC'd and FinalizationRegistry is pinged about it.

Normal: developer calls dispose, then the GC collects the Dart wrapper and pings the FinalizationRegistry about it.

Do our tests cover both situations?

We don't yet have a good way to cover an object that gets finalized without getting disposed, but @polina-c is working on that.

dnfield · 2022-09-06T16:21:04Z

The text part lgtm (modulo comments from other reviewers). But for my own edification, are all NativeFieldWrapperClasses need a dispose method now or it's case-by-case?

In general we should be disposing NativeFieldWrapperClasses, but it's not really important for ones that are very simple data classes. However, if they are just a simple data class, they probably should just be plain Dart objects to begin with.

The idea is: if it holds a large chunk of native/graphics memory, we should not be relying on GC to get that cleaned up. If it is only holding primitives that aren't that big, it's less critical to clean that up quickly but also probably never needed a C++ peer anyway.

Also there are 153 references to the TextPainter constructor in g3. By definition this is not a breaking change but to me this feels quite breaking (if developers don't update they're going to start leaking TextPainters?).

Anyone this breaks is already broken - they're relying on GC for memory clean up of possibly a single native object.

yjbanov

zanderso · 2022-09-12T16:55:47Z

SkiaPerf reported some improvements to frame build times on this change: https://flutter-flutter-perf.skia.org/e/?begin=1662484960&end=1662530119&keys=X47209473e566ccf8ce6981f790d3d282&selected=commit%3D30897%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DMoto_G__4_%252Cdevice_version%253Dandroid-24%252Chost_type%253Dlinux%252Csub_result%253D99th_percentile_frame_build_time_millis%252Ctest%253Dmulti_widget_construction_perf__e2e_summary%252C&xbaroffset=30897

dnfield · 2022-09-12T16:58:59Z

It's likely because of the fewer GC calls. We really need to add GC time totals to these too...

Dispose Paragraph objects

dfa9e61

dnfield requested review from justinmc and LongCatIsLooong August 30, 2022 22:26

missing !

6064822

dnfield mentioned this pull request Aug 30, 2022

External sizes flutter/engine#35813

Merged

Merge remote-tracking branch 'upstream/master' into text_disp

f0d4cfe

dnfield requested a review from yjbanov September 1, 2022 04:14

yjbanov reviewed Sep 1, 2022

View reviewed changes

flutter-dashboard bot added the will affect goldens Changes to golden files label Sep 1, 2022

yjbanov review

7e2fe2a

LongCatIsLooong reviewed Sep 6, 2022

View reviewed changes

dnfield requested a review from yjbanov September 6, 2022 19:36

yjbanov approved these changes Sep 6, 2022

View reviewed changes

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 6, 2022

auto-submit bot merged commit eaff156 into flutter:master Sep 6, 2022

dnfield deleted the text_disp branch September 6, 2022 20:49

This was referenced Sep 7, 2022

Roll Flutter from 759d0e1b20c1 to 58ec4839159e (93 revisions) flutter/packages#2579

Merged

Roll Flutter from 723b82e4f0f3 to 58ec4839159e (27 revisions) flutter/plugins#6370

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2022

eaff1562a Dispose Paragraph objects (flutter/flutter#110627)

b1d2e1f

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 7, 2022

eaff156 Dispose Paragraph objects (flutter/flutter#110627)

d4222a5

engine-flutter-autoroll mentioned this pull request Sep 7, 2022

Roll Flutter from 723b82e4f0f3 to 547de47a93f7 (28 revisions) flutter/plugins#6372

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 7, 2022

eaff156 Dispose Paragraph objects (flutter/flutter#110627)

7f0f52c

engine-flutter-autoroll mentioned this pull request Sep 7, 2022

Roll Flutter from 723b82e4f0f3 to ad1e76a20939 (31 revisions) flutter/plugins#6373

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 7, 2022

eaff156 Dispose Paragraph objects (flutter/flutter#110627)

229ac39

dnfield mentioned this pull request Sep 12, 2022

Add doc note about when to dispose TextPainter #111403

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dispose Paragraph objects #110627

Dispose Paragraph objects #110627

dnfield commented Aug 30, 2022

dnfield commented Aug 30, 2022

dnfield commented Aug 31, 2022

yjbanov Sep 1, 2022

dnfield Sep 3, 2022

yjbanov Sep 1, 2022

dnfield Sep 3, 2022

yjbanov Sep 1, 2022

dnfield Sep 3, 2022

yjbanov Sep 1, 2022

dnfield Sep 3, 2022

yjbanov Sep 1, 2022

dnfield Sep 3, 2022

yjbanov Sep 1, 2022

dnfield Sep 3, 2022

flutter-dashboard bot commented Sep 1, 2022

flutter-dashboard bot commented Sep 3, 2022

LongCatIsLooong left a comment

LongCatIsLooong Sep 6, 2022

dnfield Sep 6, 2022

yjbanov Sep 6, 2022

dnfield Sep 6, 2022

dnfield commented Sep 6, 2022

yjbanov left a comment

zanderso commented Sep 12, 2022

dnfield commented Sep 12, 2022

Dispose Paragraph objects #110627

Dispose Paragraph objects #110627

Conversation

dnfield commented Aug 30, 2022

dnfield commented Aug 30, 2022

dnfield commented Aug 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Sep 1, 2022

flutter-dashboard bot commented Sep 3, 2022

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Sep 6, 2022

yjbanov left a comment

Choose a reason for hiding this comment

zanderso commented Sep 12, 2022

dnfield commented Sep 12, 2022