Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Reland layer state stack 2 #37394

Merged
merged 7 commits into from
Nov 17, 2022
Merged

Conversation

flar
Copy link
Contributor

@flar flar commented Nov 7, 2022

This reland fixes the cached rendering issues (see flutter/flutter#114359) resulting from doubling up the transform when preparing a state stack for updating a cache entry.

Many of the performance regressions seem to be better now with these recent fixes, but I've been trying to track down a 10-20% regression in some of the peephole benchmarks. Most notably the "grid of opacity" benchmark (see the 2 recent bumps in this Skia Perf graph). I've tracked this down to the most probable cause being the copies and manipulations of SkM44 matrices that can be reduced or eliminated with more work.

I'm creating this PR now so that some developers can have access to the bug fixes before I fix the last of the performance regressions.

@jonahwilliams

This comment was marked as outdated.

@jonahwilliams
Copy link
Member

Actually nevermind, this is happening on ToT

@flar flar force-pushed the reland-layer-state-stack-2 branch from a15ee82 to a39d13f Compare November 14, 2022 22:54
@jonahwilliams
Copy link
Member

I did some profiling of this PR vs ToT on the regressed grid of opacity benchmark on an iPhone 6-something.

ToT Trace:
Pre-Change.trace.zip

With Change:
WithLSS.trace.zip

These profiles can be viewed by unzipping and opening in Instruments. The first few seconds are just navigation to the actual benchmark.

With a little bit of charge to callers, I can see that LayerStateStack::restore_to_count and LayerStateStack::do_save end up surprisingly high in the profile's self time. This may or may not constitute part of the reason for the regression.

LSSTrace

@flar
Copy link
Contributor Author

flar commented Nov 15, 2022

@jonahwilliams but did you see an actual regression on the iPhone? I see a net gain in performance on a Pixel 6a, so it isn't so much "that method is called a lot" unless there is something about that method that runs faster on a Pixel than on a Moto G4. Zach's guess that this is a case of blowing the smaller CPU caches on the Moto G4 due to code on different cache lines being hit makes sense, especially since much of this logic used to be duplicated into the Preroll/Paint methods. Now those methods are calling into a mechanism which may live on different code pages in memory.

@jonahwilliams
Copy link
Member

I don't really observe any regression on iOS locally, I'll try on Android in a bit.

ToT:

  "average_frame_rasterizer_time_millis": 0.812,
  "90th_percentile_frame_rasterizer_time_millis": 0.897,
  "99th_percentile_frame_rasterizer_time_millis": 1.035,
  "worst_frame_rasterizer_time_millis": 1.121,

Patch:

  "average_frame_rasterizer_time_millis": 0.828,
  "90th_percentile_frame_rasterizer_time_millis": 0.897,
  "99th_percentile_frame_rasterizer_time_millis": 1.003,
  "worst_frame_rasterizer_time_millis": 1.066,

@flar
Copy link
Contributor Author

flar commented Nov 16, 2022

I'm holding off on this while I investigate a prototype for a lower-overhead version that uses stack-allocated objects in lieue of the current vector of StateEntry objects...

@flar flar added the Work in progress (WIP) Not ready (yet) for review! label Nov 16, 2022
@flar
Copy link
Contributor Author

flar commented Nov 17, 2022

The new prototype was less wieldy than the existing design. I think I can retrofit some of the concepts back onto the existing design in a future effort, so I'm removing the WIP label on this PR.

@flar flar added autosubmit Merge PR when tree becomes green via auto submit App and removed Work in progress (WIP) Not ready (yet) for review! labels Nov 17, 2022
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@auto-submit auto-submit bot merged commit 5b31f4f into flutter:main Nov 17, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 17, 2022
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 17, 2022
…115589)

* 556e0415a Revert "[Impeller] Reland: Refactor color source resolution to use explicit factory types (#37677)" (flutter/engine#37720)

* 5b31f4f0d Reland layer state stack 2 (flutter/engine#37394)

* d955a72c5 [macOS] Merge FlutterSurfaceManager and impls (flutter/engine#37701)
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
…lutter#115589)

* 556e0415a Revert "[Impeller] Reland: Refactor color source resolution to use explicit factory types (flutter#37677)" (flutter/engine#37720)

* 5b31f4f0d Reland layer state stack 2 (flutter/engine#37394)

* d955a72c5 [macOS] Merge FlutterSurfaceManager and impls (flutter/engine#37701)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…lutter#115589)

* 556e0415a Revert "[Impeller] Reland: Refactor color source resolution to use explicit factory types (flutter#37677)" (flutter/engine#37720)

* 5b31f4f0d Reland layer state stack 2 (flutter/engine#37394)

* d955a72c5 [macOS] Merge FlutterSurfaceManager and impls (flutter/engine#37701)
auto-submit bot pushed a commit that referenced this pull request Jul 11, 2023
Fixes: flutter/flutter#82961
Fixes: flutter/flutter#113346

The fix was a simple fallout from the previous work to add support for SkM44 throughout the DL and Diff mechanisms (see flutter/flutter#82955, flutter/flutter#116198, #37394)

Tested with its own existing and new unit tests as well as the test case from flutter/flutter#113346
kjlubick pushed a commit to kjlubick/engine that referenced this pull request Jul 14, 2023
Fixes: flutter/flutter#82961
Fixes: flutter/flutter#113346

The fix was a simple fallout from the previous work to add support for SkM44 throughout the DL and Diff mechanisms (see flutter/flutter#82955, flutter/flutter#116198, flutter#37394)

Tested with its own existing and new unit tests as well as the test case from flutter/flutter#113346
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants