Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] when creating new pipeline variant block on current thread, re-persist dirty pipeline cache. #52375

Merged
merged 3 commits into from
Apr 28, 2024

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Apr 24, 2024

Current diagram of how PSO variants are created (after first bootstrap)

Raster thread.                             Worker Thread
1.  Check Pipeline Cache
2. Pipeline Cache Empty
3. Create Promise
4. Spawn Worker                     ->      Compile Pipeline
5. Block On Future.                       ..                                                    
6.
7. Resolve                          <-      Complete Future

This is a serialized workload due to blocking on future completion. But performing it on two threads means that we're also adding in the somewhat random cost of thread scheduling and or getting deprioritized.

Instead when creating variants, block on the current thread. Now if we knew all the variants we need to create for a frame ahead of time, we could spawn one or more works to creat them more quickly. This would require more work though.

While I'm at it. Update the pipeline cache to persist every 50 frames, if it has added a new shader.

part of flutter/flutter#129660

@jonahwilliams
Copy link
Member Author

LMK if this makes any sense. I don't think it will be faster on average, but there can definitely be cases where the phone is busy and it takes a while for the task to get scheduled.

@chinmaygarde
Copy link
Member

somewhat random cost of thread scheduling and or getting deprioritized

My sense was that the cost of 1) small enough to not be statistically significant, and 2) didn't happen all that often because the variant was in the map already.

But I suppose I am somewhat convinced this could be an issue. I am suggest an alternative method call instead of the GetPipeline. GetPipelineSync or GetPipelineBlocking or GetPipelineNow. Instead of a default argument, this will be more self documenting IMO. LGTM otherwise.

@jonahwilliams
Copy link
Member Author

the scheduling is generally fine on iOS but appears to be a crapshoot on Android.

@chinmaygarde
Copy link
Member

Yeah, makes sense. And I guess we will toss the task onto a concurrent loop whose priorities may be different.

@jonahwilliams
Copy link
Member Author

priorities /affinities don't really matter, you can't guarantee that code will be run immediately with the task runners on any platform. Android is busier than iOS but the same thing can happen there if we're doing a lot of image decoding.

GetPipeline. GetPipelineSync or GetPipelineBlocking or GetPipelineNow. Instead of a default argument, this will be more self documenting IMO. LGTM otherwise.

There is a pretty long chain of methods from create variant, through get pipeline, into the pipeline caches. One of the reasons I used the argument was that I could leave most of this as is without duplicating the methods to have sync/async versions down to the pipelin cache.

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 28, 2024
@auto-submit auto-submit bot merged commit dbeec53 into flutter:main Apr 28, 2024
29 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 28, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 28, 2024
…147495)

flutter/engine@752b146...f4c20e9

2024-04-28 [email protected] [Impeller] remove image upload from IO thread, limit concurrent worker threads. (flutter/engine#52423)
2024-04-28 [email protected] [Impeller] when creating new pipeline variant block on current thread, re-persist dirty pipeline cache. (flutter/engine#52375)

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:/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
auto-submit bot added a commit to flutter/flutter that referenced this pull request Apr 29, 2024
…isions) (#147495)" (#147502)

Reverts: #147495
Initiated by: zanderso
Reason for reverting: Crash in framework CI https://ci.chromium.org/ui/p/flutter/builders/prod/Linux_pixel_7pro%20new_gallery_opengles_impeller__transition_perf/2417/overview
Original PR Author: engine-flutter-autoroll

Reviewed By: {fluttergithubbot}

This change reverts the following previous change:

flutter/engine@752b146...f4c20e9

2024-04-28 [email protected] [Impeller] remove image upload from IO thread, limit concurrent worker threads. (flutter/engine#52423)
2024-04-28 [email protected] [Impeller] when creating new pipeline variant block on current thread, re-persist dirty pipeline cache. (flutter/engine#52375)

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:/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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 29, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 29, 2024
…147532)

flutter/engine@752b146...399837d

2024-04-29 [email protected] Roll Skia from 27e872349963 to f7bfa8eef5b5 (1 revision) (flutter/engine#52436)
2024-04-29 [email protected] [Impeller] fix GLES image upload. (flutter/engine#52430)
2024-04-29 [email protected] Roll Skia from aeab79038011 to 27e872349963 (1 revision) (flutter/engine#52435)
2024-04-29 [email protected] Roll Skia from c720e2446926 to aeab79038011 (1 revision) (flutter/engine#52434)
2024-04-29 [email protected] Roll Skia from 1a5436d50954 to c720e2446926 (1 revision) (flutter/engine#52433)
2024-04-29 [email protected] Roll Fuchsia Linux SDK from bIUvi3y4gRFxMSKV3... to TFm2_qWC2xpkzk8QS... (flutter/engine#52432)
2024-04-29 [email protected] Roll Skia from e3dfcd1b25af to 1a5436d50954 (1 revision) (flutter/engine#52431)
2024-04-28 [email protected] Use a AT-SPI socket/plug to export the Flutter accessibility state. (flutter/engine#52355)
2024-04-28 [email protected] [Impeller] remove image upload from IO thread, limit concurrent worker threads. (flutter/engine#52423)
2024-04-28 [email protected] [Impeller] when creating new pipeline variant block on current thread, re-persist dirty pipeline cache. (flutter/engine#52375)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from bIUvi3y4gRFx to TFm2_qWC2xpk

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:/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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants