[display_list] paint cleanup. by jonahwilliams · Pull Request #168082 · flutter/flutter

jonahwilliams · 2025-04-30T18:52:07Z

Reduce the number of places where we convert flutter paint to DLpaint and remove unused tonic overrides. Also does a pass on dl_paint/builder to remove places we accidentally incremented/decrement shared_ptr ref counts

jonahwilliams · 2025-04-30T18:52:33Z

engine/src/flutter/display_list/dl_builder.cc

  }
  if (flags.applies_shader()) {
-    setColorSource(paint.getColorSource().get());
+    setColorSource(paint.getColorSourcePtr());


Use the ptr APIs directly so we don't increment + decrement the shared_ptr

Doesn't it have a shared_ptr interface to avoid the copy? Wouldn't that be better?

I don't think it matters since we're not storing anything.

Ah, I see you are just now adding the shared_ptr setters. Woot!

Also, builder often copies the object to inline it anyway, but maybe we should reconsider that? I guess if ColorSource objects tend to be fast-copy then it's OK, but otherwise, storing a shared_ptr to the original would be desireable and that can't happen when you've done Ptr().

These could also be made shared_from_this compatible.

(Once upon a time I created a better sk_sp as dl_sp and it solved all of these issues. ;)

The issue here is that setColorSource can call onSetColorSource if the objects are different which needs a shared object. This should have some more refactoring otherwise we are recreating the object inside onSetColorSource.

We need setColorSource(ptr) because builder implements DlOpReceiver for copying of data. I might be able to get rid of that eventually, but until then perhaps the SetAttriutesFrom method should do its own comparisons and onSetColorSource takes a shared_ptr (ref). Thus, the DlOpReceiver version might need to call shared(), but not the one that syncs with a paint. Another issue is that if it is null then you can't call shared(). :(

hmm, yeah we want to avoid calling shared_from_this if possible since that does a weak ptr lock. Instead I'll change the setCall to take a refer and then we can avoid the shared call when non-null.

There is no shared_from_this currently. We have shared() which instantiates a whole new copy inside a shared_ptr. It's even worse than a weak ptr lock...?

eh, then it will be fine. I could keep refactoring this for a while but since I'm about to go OOO maybe call this good enough?

I have no objections and @gaaclarke already approved it. I think these changes move us forward.

jonahwilliams · 2025-04-30T18:52:52Z

engine/src/flutter/display_list/dl_builder.cc


  if (flags.applies_mask_filter()) {
-    auto filter = current_.getMaskFilter();
+    const DlMaskFilter* filter = current_.getMaskFilterPtr();


Only need the ptr here, same as usages for image/color filter below.

jonahwilliams · 2025-04-30T18:53:06Z

engine/src/flutter/display_list/dl_builder.h

  // |DlOpReceiver|
  void setColorSource(const DlColorSource* source) override {
-    if (NotEquals(current_.getColorSource(), source)) {
+    if (NotEquals(current_.getColorSourcePtr(), source)) {


compare the ptrs so we avoid the shared_ptr increment/decrement.

jonahwilliams · 2025-04-30T18:53:26Z

engine/src/flutter/display_list/dl_paint.h

  }

-  std::shared_ptr<const DlColorSource> getColorSource() const {
+  const std::shared_ptr<const DlColorSource>& getColorSource() const {


return a ref so that callers that specify a const ref don't increment/decrement.

Isn't this sufficient alone to get rid of all those increment decrements, so the getColorSourcePtr calls aren't required?

in theory, yes, but since we were calling getColorSource().get() anyway its still an improvement imo

Yeah, get().get() should be outlawed.

jonahwilliams · 2025-04-30T18:54:08Z

engine/src/flutter/display_list/dl_paint.h

-  DlPaint& setColorSource(std::shared_ptr<const DlColorSource> source) {
-    color_source_ = std::move(source);
+
+  DlPaint& setColorSource(std::nullptr_t source) {


Add nullptr_t overloads. Overload resolution for passing nullptr literal is ambiguous between selecting the raw ptr or shared_ptr. If the shared_ptr path is selected we will construct and destroy a shared_ptr.

jonahwilliams · 2025-04-30T18:54:35Z

engine/src/flutter/lib/ui/painting/paint.cc

  return &paint;
 }

-void Paint::toDlPaint(DlPaint& paint, DlTileMode tile_mode) const {


This method is almost the same as the above conversion, expect it doesn't use flags. Remove it and give the draw paragraph code some flags.

jonahwilliams · 2025-04-30T18:54:45Z

engine/src/flutter/lib/ui/painting/paint.h

-
 }  // namespace flutter

-namespace tonic {


jonahwilliams · 2025-04-30T18:54:56Z

engine/src/flutter/lib/ui/painting/paint_unittests.cc

    Paint ui_paint(paint_objects, paint_data);
-    ui_paint.toDlPaint(dl_paint, DlTileMode::kClamp);
+
+    ui_paint.paint(dl_paint, DisplayListOpFlags::kDrawPaintFlags,


Lets test the API that is actually used for most draws...

gaaclarke · 2025-04-30T19:35:28Z

engine/src/flutter/display_list/dl_paint.h

  }

-  std::shared_ptr<const DlColorSource> getColorSource() const {
+  const std::shared_ptr<const DlColorSource>& getColorSource() const {


Isn't this sufficient alone to get rid of all those increment decrements, so the getColorSourcePtr calls aren't required?

flar · 2025-04-30T19:47:14Z

engine/src/flutter/display_list/dl_paint.h

    color_source_ = source ? source->shared() : nullptr;
    return *this;
  }
+  DlPaint& setColorSource(std::shared_ptr<const DlColorSource> source) {


What if this took a shared_ptr&?

There's the same number of copies in the worst case, in the best case (where someone has an rvalue in setColorSource) it's less.

(the current implementation)

flar · 2025-04-30T21:15:35Z

I think this eliminates some of the ref count work. I'm happy if @gaaclarke is happy, but I think this entire mechanism should be reworked internally because when the attribute changes we end up going through Ptr->needs shptr->calls shared(). and that could/should be eliminated.

flar · 2025-04-30T21:26:28Z

Another unnecessary inc:

flutter/engine/src/flutter/txt/src/skia/paragraph_skia.cc

Line 183 in adae8bb

return paint.getColorSource() ||

jonahwilliams · 2025-04-30T22:23:58Z

That call should be fine since we're just checking the const ref, no increment/decrement.

auto-submit · 2025-05-01T22:55:08Z

autosubmit label was removed for flutter/flutter/168082, because - The status or check suite Linux customer_testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2025-05-14T17:11:25Z

autosubmit label was removed for flutter/flutter/168082, because - The status or check suite Windows web_tool_tests_1_2 has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2025-05-14T18:41:32Z

autosubmit label was removed for flutter/flutter/168082, because - The status or check suite Windows web_tool_tests_1_2 has failed. Please fix the issues identified (or deflake) before re-applying this label.

flutter/flutter@0b9f928...9a78af5 2025-05-15 [email protected] Manual pub package roll (flutter/flutter#168916) 2025-05-15 [email protected] Remove unnecessary `bringup: true` for release-channel only `Linux flutter_packaging`. (flutter/flutter#168761) 2025-05-15 [email protected] Revert: "Run `flutter_packaging` builders on release candidates (flutter/flutter#168917) 2025-05-15 [email protected] Roll Dart SDK from a6c25e31caa7 to c9640c3a4440 (1 revision) (flutter/flutter#168911) 2025-05-15 [email protected] Roll Packages from 1468581 to 2dff621 (4 revisions) (flutter/flutter#168908) 2025-05-15 [email protected] Roll Dart SDK from b3520981e0f0 to a6c25e31caa7 (11 revisions) (flutter/flutter#168895) 2025-05-15 [email protected] Roll Fuchsia Linux SDK from fSvuEJgRmHxnewRJr... to Jj-iDG5uPOsFgY2_H... (flutter/flutter#168893) 2025-05-15 [email protected] Fix mac_ios_engine_ddm config with missing ci/ios_debug_sim_ddm config (flutter/flutter#168888) 2025-05-15 [email protected] [native assets] Remove `KernelSnapshot` dependency in build (flutter/flutter#168742) 2025-05-15 [email protected] iOS,macOS: Migrate logging to Logger/FlutterLogger (flutter/flutter#168568) 2025-05-15 [email protected] Skip hot reload breakpoints test when running with web (flutter/flutter#168873) 2025-05-15 [email protected] CupertinoSliverNavigationBar respects accessibility text scaling (flutter/flutter#168866) 2025-05-15 [email protected] [display_list] paint cleanup. (flutter/flutter#168082) 2025-05-15 [email protected] Add a new CI build for iOS DDM-enabled artifacts (flutter/flutter#168717) 2025-05-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Implements UISceneDelegate dynamically w/ FlutterLaunchEngine (#168396)" (flutter/flutter#168880) 2025-05-14 [email protected] Implements UISceneDelegate dynamically w/ FlutterLaunchEngine (flutter/flutter#168396) 2025-05-14 [email protected] Mark web_tool_tests_1_2 as bringup. (flutter/flutter#168871) 2025-05-14 [email protected] Marks Mac_mokey run_debug_test_android to be unflaky (flutter/flutter#167634) 2025-05-14 [email protected] Reland "Clip search artifacts in CupertinoSliverNavigationBar searchable-to-searchable transitions" (flutter/flutter#168772) 2025-05-14 [email protected] Remove references to `team-release`. (flutter/flutter#168780) 2025-05-14 [email protected] Make Cupertino sheet set the systemUIStyle through an AnnotatedRegion (flutter/flutter#168182) 2025-05-14 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Use live region in error text input decorator for Android (#165531)" (flutter/flutter#168848) 2025-05-14 [email protected] Normalize BottomAppBarTheme (flutter/flutter#168586) 2025-05-14 [email protected] Roll Packages from 2e166de to 1468581 (2 revisions) (flutter/flutter#168828) 2025-05-14 [email protected] macOS,iOS: fix swift target triple (flutter/flutter#168749) 2025-05-14 [email protected] Further update `Engine-artifacts.md`. (flutter/flutter#168779) 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] 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

[display_list] paint cleanup.

9c4fe9d

github-actions bot added the engine flutter/engine related. See also e: labels. label Apr 30, 2025

jonahwilliams commented Apr 30, 2025

View reviewed changes

engine/src/flutter/lib/ui/painting/paint.h

} // namespace flutter

namespace tonic {

Copy link

Contributor Author

jonahwilliams Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused

jonahwilliams commented Apr 30, 2025

View reviewed changes

jonahwilliams marked this pull request as ready for review April 30, 2025 18:55

jonahwilliams requested review from flar and gaaclarke April 30, 2025 18:55

gaaclarke approved these changes Apr 30, 2025

View reviewed changes

flar reviewed Apr 30, 2025

View reviewed changes

jonahwilliams added 2 commits April 30, 2025 12:59

fix unit tests.

158287b

Merge branch 'master' into ptr_cleanup

46e7c6f

Merge branch 'master' into ptr_cleanup

3d0a578

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label May 1, 2025

Merge branch 'master' into ptr_cleanup

3e6c117

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 1, 2025

Merge branch 'master' into ptr_cleanup

6f9d5a1

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label May 14, 2025

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 14, 2025

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label May 14, 2025

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 14, 2025

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label May 14, 2025

auto-submit bot added this pull request to the merge queue May 15, 2025

Merged via the queue into flutter:master with commit b443c2a May 15, 2025
184 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 15, 2025

engine-flutter-autoroll mentioned this pull request May 15, 2025

Roll Flutter from 0b9f9288caf3 to 1d8078552d99 (21 revisions) flutter/packages#9253

Closed

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

b443c2a [display_list] paint cleanup. (flutter/flutter#168082)

69a1c00

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

b443c2a [display_list] paint cleanup. (flutter/flutter#168082)

6655a9d

engine-flutter-autoroll mentioned this pull request May 15, 2025

Roll Flutter from 0b9f9288caf3 to d69edc303b49 (22 revisions) flutter/packages#9255

Closed

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

b443c2a [display_list] paint cleanup. (flutter/flutter#168082)

a5e5771

engine-flutter-autoroll mentioned this pull request May 15, 2025

Roll Flutter from 0b9f9288caf3 to a164f6538603 (23 revisions) flutter/packages#9256

Closed

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

b443c2a [display_list] paint cleanup. (flutter/flutter#168082)

1d1ebd5

engine-flutter-autoroll mentioned this pull request May 15, 2025

Roll Flutter from 0b9f9288caf3 to 94c661082cc3 (24 revisions) flutter/packages#9257

Closed

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

b443c2a [display_list] paint cleanup. (flutter/flutter#168082)

a84f2a9

engine-flutter-autoroll mentioned this pull request May 15, 2025

Roll Flutter from 0b9f9288caf3 to 9214ed12dbae (25 revisions) flutter/packages#9258

Closed

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

b443c2a [display_list] paint cleanup. (flutter/flutter#168082)

f8cd5b7

engine-flutter-autoroll mentioned this pull request May 15, 2025

Roll Flutter from 0b9f9288caf3 to 9a78af5eb067 (26 revisions) flutter/packages#9259

Merged

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

b443c2a [display_list] paint cleanup. (flutter/flutter#168082)

b08b22c

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

b443c2a [display_list] paint cleanup. (flutter/flutter#168082)

2ef40ea

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025

b443c2a [display_list] paint cleanup. (flutter/flutter#168082)

3600225

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025

b443c2a [display_list] paint cleanup. (flutter/flutter#168082)

68434d4

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

b443c2a [display_list] paint cleanup. (flutter/flutter#168082)

33ca3a6

Conversation

jonahwilliams commented Apr 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flar commented Apr 30, 2025

Uh oh!

flar commented Apr 30, 2025

Uh oh!

jonahwilliams commented Apr 30, 2025

Uh oh!

auto-submit bot commented May 1, 2025

Uh oh!

auto-submit bot commented May 14, 2025

Uh oh!

auto-submit bot commented May 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants