[Impeller] Reorder pipeline construction in content context. #54373

chinmaygarde · 2024-08-06T20:21:55Z

Fixes the regression to the first-frame time on the Mokey.

Also add a trace that should allow for easier identification of problematic pipelines.

A more involved job-queue that allows construction jobs to skip the queue was written in #54355. But the Mokey case is not important enough for the additional complexity. See the context in the linked patch.

start_up_timeline.json.zip

Fixes the regression to the first-frame time on the Mokey. Also add a trace that should allow for easier identification of problematic pipelines. A more involved job-queue that allows construction jobs to skip the queue was written in flutter#54355. But the Mokey case is not important enough for the additional complexity. See the context in the linked patch.

flutter-dashboard · 2024-08-06T20:21:58Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

gaaclarke

LGTM modulo sorting these together with the other high priority pipelines

gaaclarke · 2024-08-06T20:24:33Z

impeller/entity/contents/content_context.cc

+  glyph_atlas_pipelines_.CreateDefault(
+      *context_, options,
+      {static_cast<Scalar>(
+          GetContext()->GetCapabilities()->GetDefaultGlyphAtlasFormat() ==
+          PixelFormat::kA8UNormInt)});
+  texture_downsample_pipelines_.CreateDefault(*context_, options_trianglestrip);
+  rrect_blur_pipelines_.CreateDefault(*context_, options_trianglestrip);
+  texture_strict_src_pipelines_.CreateDefault(*context_, options);
+  tiled_texture_pipelines_.CreateDefault(*context_, options, {supports_decal});
+  gaussian_blur_pipelines_.CreateDefault(*context_, options_trianglestrip,
+                                         {supports_decal});
+  border_mask_blur_pipelines_.CreateDefault(*context_, options_trianglestrip);
+  color_matrix_color_filter_pipelines_.CreateDefault(*context_,
+                                                     options_trianglestrip);
+  porter_duff_blend_pipelines_.CreateDefault(*context_, options_trianglestrip,
+                                             {supports_decal});
+  vertices_uber_shader_.CreateDefault(*context_, options, {supports_decal});
+


Can you put these in the brackets above? That's what that section is for, higher priority pipelines. Maybe I should have added a comment there last time I sorted these.

Not all of these shaders should move up top though. The re-ordering of these is fine, but the glyph atlas one should move of

Oh, is that what that was for. I'll move and add the comment.

but the glyph atlas one should move of

Elaborate?

oops. "up" I mean. Like we should create glyph atlas after solid color et cetera.

Oh, I found a few other cases like the rrect blur stuff in the gallery. So at least some of this necessary IMO.

The changes you made are fine, I'm suggesting moving the glyph atlas up even higher.

Derp. Ok ok. Got it.

jonahwilliams

LGTM

flutter/engine#54373)

…152985) flutter/engine@206e86e...5a0fd5f 2024-08-07 [email protected] [Impeller] Statically check stage interfaces for precision mismatches. (flutter/engine#54375) 2024-08-07 [email protected] Roll Skia from f04ba9cd0e4b to 968a00456bc5 (4 revisions) (flutter/engine#54378) 2024-08-06 [email protected] Roll Dart SDK from dedfa3393296 to f5d136899082 (5 revisions) (flutter/engine#54376) 2024-08-06 [email protected] [Impeller] Reorder pipeline construction in content context. (flutter/engine#54373) 2024-08-06 [email protected] [iOS] clean ups to platform view controller (flutter/engine#54335) 2024-08-06 [email protected] [Impeller] ensure precision matches for buggy vulkan drivers. (flutter/engine#54372) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll 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 Flutter: 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

…lutter#152985) flutter/engine@206e86e...5a0fd5f 2024-08-07 [email protected] [Impeller] Statically check stage interfaces for precision mismatches. (flutter/engine#54375) 2024-08-07 [email protected] Roll Skia from f04ba9cd0e4b to 968a00456bc5 (4 revisions) (flutter/engine#54378) 2024-08-06 [email protected] Roll Dart SDK from dedfa3393296 to f5d136899082 (5 revisions) (flutter/engine#54376) 2024-08-06 [email protected] [Impeller] Reorder pipeline construction in content context. (flutter/engine#54373) 2024-08-06 [email protected] [iOS] clean ups to platform view controller (flutter/engine#54335) 2024-08-06 [email protected] [Impeller] ensure precision matches for buggy vulkan drivers. (flutter/engine#54372) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll 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 Flutter: 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

chinmaygarde requested review from gaaclarke and jonahwilliams August 6, 2024 20:21

github-actions bot added the e: impeller label Aug 6, 2024

chinmaygarde mentioned this pull request Aug 6, 2024

[Impeller] Allow jumpstarts to pipeline readiness. #54355

Closed

gaaclarke approved these changes Aug 6, 2024

View reviewed changes

PR

5652503

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 6, 2024

jonahwilliams approved these changes Aug 6, 2024

View reviewed changes

auto-submit bot merged commit 85d989e into flutter:main Aug 6, 2024

chinmaygarde deleted the reorder branch August 6, 2024 22:39

engine-flutter-autoroll mentioned this pull request Aug 6, 2024

Roll Flutter Engine from 206e86ee8a40 to 85d989ed2b23 (3 revisions) flutter/flutter#152973

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 6, 2024

85d989ed2 [Impeller] Reorder pipeline construction in content context. (

46c27bc

flutter/engine#54373)

engine-flutter-autoroll mentioned this pull request Aug 7, 2024

Roll Flutter Engine from 206e86ee8a40 to c20371dab213 (4 revisions) flutter/flutter#152975

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 7, 2024

85d989ed2 [Impeller] Reorder pipeline construction in content context. (

ceb4939

flutter/engine#54373)

engine-flutter-autoroll mentioned this pull request Aug 7, 2024

Roll Flutter Engine from 206e86ee8a40 to 5a0fd5fbecc6 (6 revisions) flutter/flutter#152980

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 7, 2024

85d989ed2 [Impeller] Reorder pipeline construction in content context. (

a7afcb5

flutter/engine#54373)

engine-flutter-autoroll mentioned this pull request Aug 7, 2024

Roll Flutter Engine from 206e86ee8a40 to 5a0fd5fbecc6 (6 revisions) flutter/flutter#152985

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 7, 2024

85d989ed2 [Impeller] Reorder pipeline construction in content context. (

fbc6f42

flutter/engine#54373)

[Impeller] Reorder pipeline construction in content context. #54373

[Impeller] Reorder pipeline construction in content context. #54373

Uh oh!

Conversation

chinmaygarde commented Aug 6, 2024

Uh oh!

flutter-dashboard bot commented Aug 6, 2024

Uh oh!

gaaclarke left a comment

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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants