Add dump-shader-skp switch to help ShaderWarmUp #8148

liyuqian · 2019-03-13T00:09:02Z

Allow Flutter to automatically dump the skp that triggers new shader compilations. This is useful for writing custom ShaderWarmUp to reduce jank. By default, it's not enabled to reduce the overhead. This is only available in profile or debug build.

Later, we can add service protocol support to pull the skp from the client to the host. Currently, it works fine for Android-based devices (including our urgent internal clients) where we can adb shell into the cache directory.

matthew-carroll · 2019-03-13T00:11:42Z

shell/platform/android/io/flutter/embedding/engine/FlutterShellArgs.java

@@ -37,6 +37,8 @@
  public static final String ARG_SKIA_DETERMINISTIC_RENDERING = "--skia-deterministic-rendering";
  public static final String ARG_KEY_TRACE_SKIA = "trace-skia";
  public static final String ARG_TRACE_SKIA = "--trace-skia";
+  public static final String ARG_KEY_DUMP_SHADER_SKP = "dump-shader-skp";


Do these need to be pulled out of the Intent in fromIntent()?

Good catch! Fixed.

Interestingly, we also have a similar logic in FlutterActivityDelegate.java. What's the difference between the two?

FlutterActivityDelegate is a mechanism in the old version of the embedding. FlutterShellArgs is part of the new embedding.

jason-simmons · 2019-03-13T00:22:06Z

shell/common/rasterizer.cc

  if (DrawToSurface(*layer_tree)) {
    last_layer_tree_ = std::move(layer_tree);
  }
+
+#if FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_RELEASE
+  if (persistent_cache->IsDumpingSkp() && persistent_cache->IsAccessed()) {


Instead of the #ifdefs, can all of this just be made conditional on IsDumpingSkp (which presumably will be false in release mode)

I agree. Let's make sure all the code compiles in all configurations. LTO should get rid of the unreachable code anyway. I also followed the same pattern in the raster cache which I will correct.

Ok, then I'll remove those #ifdefs assuming that there won't be a big impact on our binary code size. @goderbauer : if you find any significant regression related to this PR, please let me know and I'm happy to either revert it or add the #ifdefs back.

chinmaygarde · 2019-03-13T00:24:24Z

shell/common/persistent_cache.cc

+
+  std::stringstream name_stream;
+  int current_index = skp_index_++;
+  skp_index_ %= kMaxSkpIndex;


Why limit the maximum number of pictures captured? I'd suggest appending the timestamp to the end of the skp as well (std::to_string(fml::TimePoint::Now() or something)). Just so its in increasing order. This is a profiling feature anyway so the disk usage is not a concern.

chinmaygarde · 2019-03-13T00:29:17Z

shell/common/persistent_cache.cc

+  skp_index_ %= kMaxSkpIndex;
+  name_stream << "shader_dump_" << current_index << ".skp";
+  std::string file_name = name_stream.str();
+  FML_LOG(INFO) << "Dumping " << file_name;


Did you mean to leave this log in here? We don't display this log level by default.

Yes, I see that only --verbose-logging will display such logs. I'll just leave it here and either send --verbose-logging manually or wait flutter/flutter#29320 to be fixed 😃

chinmaygarde · 2019-03-13T00:33:37Z

shell/common/persistent_cache.cc

@@ -81,6 +81,7 @@ bool PersistentCache::IsValid() const {
 // |GrContextOptions::PersistentCache|
 sk_sp<SkData> PersistentCache::load(const SkData& key) {
  TRACE_EVENT0("flutter", "PersistentCacheLoad");
+  is_accessed_ = true;


Aren't we only interested in pictures where a persistent cache entry is stored? In cases of cache loads, we are already reusing something that we have seen (and cached) before. My anticipation of the logic was:

Skia asks us to store shaders for a new picture we have never seen before.

We store the skp for that picture.

Thinking about this some more: Maybe you didn't mean for this feature to say "give me the SKP dumps of new pictures that cause shader compilations", but instead, "give me all SKP dumps of pictures that already caused shader compilations". If the latter is the case, just look for all SKPs on disk. Either way, the is_accessed logic I think can be made more elegant.

Done. Changed the flag to switch on store instead on load.

This is the accompanying change for flutter/engine#8148 and it needs the engine PR to land first.

…ne#8148)

flutter/engine@403337e...b36068c git log 403337e..b36068c --no-merges --oneline b36068c Roll src/third_party/skia 05201fc7e77e..aefecad7c0d2 (5 commits) (flutter/engine#8176) b32d0ab Bugfix: Prevent crash when responding to a platform message after FlutterJNI detaches from native (#28651). (flutter/engine#8170) d9b2f09 Roll src/third_party/skia 512e38091c85..05201fc7e77e (10 commits) (flutter/engine#8173) 4b01d79 Add frame and target time metadata to vsync events and connect platform vsync events using flows. (flutter/engine#8172) f7a0922 [fuchsia] Remove deprecated libraries from snapshot (flutter/engine#8085) 09db84f Android Embedding PR 19: Add accessibility to new FlutterView. (flutter/engine#8109) 246f0e3 Add an allocator specific check to ensure that strings passed to the timeline are not heap allocated. (flutter/engine#8168) 7a6bb99 Roll src/third_party/skia feb720f746dc..512e38091c85 (11 commits) (flutter/engine#8169) 5825bda Roll src/third_party/dart 7d560f8385..7418238239 (61 commits) 66fdeb1 Add dump-shader-skp switch to help ShaderWarmUp (flutter/engine#8148) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (jsimmons@google.com), and stop the roller if necessary.

This is the accompanying change for flutter/engine#8148 and it needs the engine PR to land first. For #813

Allow Flutter to automatically dump the skp that triggers new shader compilations. This is useful for writing custom ShaderWarmUp to reduce jank. By default, it's not enabled to reduce the overhead. This is only available in profile or debug build. Later, we can add service protocol support to pull the skp from the client to the host. Currently, it works fine for Android-based devices (including our urgent internal clients) where we can `adb shell` into the cache directory.

This reverts commit 0c6de52.

liyuqian requested review from chinmaygarde, cbracken, matthew-carroll and jason-simmons March 13, 2019 00:09

googlebot added the cla: yes label Mar 13, 2019

Add dump-shader-skp switch to help ShaderWarmUp

a1949db

liyuqian force-pushed the dump_skp branch from eded5dc to a1949db Compare March 13, 2019 00:10

matthew-carroll reviewed Mar 13, 2019

View reviewed changes

liyuqian mentioned this pull request Mar 13, 2019

Add dump-shader-skp flag to flutter tools flutter/flutter#29258

Merged

10 tasks

jason-simmons reviewed Mar 13, 2019

View reviewed changes

chinmaygarde reviewed Mar 13, 2019

View reviewed changes

liyuqian added a commit to liyuqian/flutter that referenced this pull request Mar 13, 2019

Add dump-shader-skp flag to flutter tools

353b467

This is the accompanying change for flutter/engine#8148 and it needs the engine PR to land first.

liyuqian added 3 commits March 13, 2019 11:56

Remove #if

08bcff2

Fix missing statement in fromIntent

b5198bb

Better name

e46ec4b

liyuqian mentioned this pull request Mar 13, 2019

add option for --verbose-system-logs flutter/flutter#29321

Merged

10 tasks

chinmaygarde approved these changes Mar 14, 2019

View reviewed changes

liyuqian merged commit 66fdeb1 into flutter:master Mar 14, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 14, 2019

66fdeb1 Add dump-shader-skp switch to help ShaderWarmUp (flutter/engi…

82f66e1

…ne#8148)

engine-flutter-autoroll mentioned this pull request Mar 14, 2019

Roll engine 8b1a299ed52d..66fdeb163ef6 (17 commits) flutter/flutter#29388

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 14, 2019

66fdeb1 Add dump-shader-skp switch to help ShaderWarmUp (flutter/engi…

b83cf0c

…ne#8148)

engine-flutter-autoroll mentioned this pull request Mar 14, 2019

Roll engine 8b1a299ed52d..5825bdac92cd (18 commits) flutter/flutter#29391

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 14, 2019

66fdeb1 Add dump-shader-skp switch to help ShaderWarmUp (flutter/engi…

becfc2a

…ne#8148)

engine-flutter-autoroll mentioned this pull request Mar 14, 2019

Roll engine 8b1a299ed52d..7a6bb9998ed4 (19 commits) flutter/flutter#29393

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 14, 2019

66fdeb1 Add dump-shader-skp switch to help ShaderWarmUp (flutter/engi…

da59f15

…ne#8148)

engine-flutter-autoroll mentioned this pull request Mar 14, 2019

Roll engine 8b1a299ed52d..246f0e3f4ab4 (20 commits) flutter/flutter#29394

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 14, 2019

66fdeb1 Add dump-shader-skp switch to help ShaderWarmUp (flutter/engi…

df22390

…ne#8148)

This was referenced Mar 14, 2019

Roll engine 8b1a299ed52d..09db84fad629 (21 commits) flutter/flutter#29397

Closed

Roll engine 8b1a299ed52d..f7a092262cf3 (22 commits) flutter/flutter#29400

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2019

66fdeb1 Add dump-shader-skp switch to help ShaderWarmUp (flutter/engi…

a1a86a0

…ne#8148)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2019

66fdeb1 Add dump-shader-skp switch to help ShaderWarmUp (flutter/engi…

7f6856c

…ne#8148)

This was referenced Mar 15, 2019

Roll engine 8b1a299ed52d..4b01d795feec (23 commits) flutter/flutter#29402

Closed

Roll engine 403337ebb893..b36068c3778c (10 commits) flutter/flutter#29429

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2019

66fdeb1 Add dump-shader-skp switch to help ShaderWarmUp (flutter/engi…

cd3eece

…ne#8148)

liyuqian added a commit to flutter/flutter that referenced this pull request Mar 15, 2019

Add dump-shader-skp flag to flutter tools (#29258)

be5c83b

This is the accompanying change for flutter/engine#8148 and it needs the engine PR to land first. For #813

RBogie added a commit to RBogie/flutter-engine that referenced this pull request Apr 8, 2019

Revert "Add dump-shader-skp switch to help ShaderWarmUp (flutter#8148)"

d4538b6

This reverts commit 0c6de52.

RBogie added a commit to RBogie/flutter-engine that referenced this pull request Apr 8, 2019

Revert "Add dump-shader-skp switch to help ShaderWarmUp (flutter#8148)"

9a11a03

This reverts commit 0c6de52.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dump-shader-skp switch to help ShaderWarmUp #8148

Add dump-shader-skp switch to help ShaderWarmUp #8148

liyuqian commented Mar 13, 2019 •

edited

Loading

matthew-carroll Mar 13, 2019

liyuqian Mar 13, 2019

matthew-carroll Mar 13, 2019

jason-simmons Mar 13, 2019

chinmaygarde Mar 13, 2019

liyuqian Mar 13, 2019

chinmaygarde Mar 13, 2019

liyuqian Mar 13, 2019

chinmaygarde Mar 13, 2019

liyuqian Mar 13, 2019

chinmaygarde Mar 13, 2019

chinmaygarde Mar 13, 2019 •

edited

Loading

liyuqian Mar 13, 2019

Add dump-shader-skp switch to help ShaderWarmUp #8148

Add dump-shader-skp switch to help ShaderWarmUp #8148

Conversation

liyuqian commented Mar 13, 2019 • edited Loading

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

chinmaygarde Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liyuqian commented Mar 13, 2019 •

edited

Loading

chinmaygarde Mar 13, 2019 •

edited

Loading