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.

Sign up for GitHub

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

Jump to bottom

[flutter_runner] Enable Skia tracing by default on Fuchsia #13457

Merged
merged 1 commit into from
Dec 12, 2019
Merged

[flutter_runner] Enable Skia tracing by default on Fuchsia #13457

merged 1 commit into from
Dec 12, 2019
+2 −3

Conversation

nathanrogersgoogle
Copy link
Contributor

@nathanrogersgoogle nathanrogersgoogle commented Oct 31, 2019 •
edited
Loading

Since Flutter tracing is wired up to Fuchsia system level tracing (and
that includes Skia tracing within Flutter), it makes more sense to
enable Skia tracing by default on Fuchsia, and to control Flutter Skia
tracing, rely on whether Fuchsia system tracing is enabled, in progress,
and contains the "skia" category.

@liyuqian
Copy link
Contributor

@nathanrogersgoogle : the engine repo is checked out in fuchsia under fuchsia/third_party/flutter. I think you can modify the code there to test locally.

@nathanrogersgoogle
Copy link
Contributor Author

I also noticed the fuchsia/third_party/flutter hanging out in the Fuchsia tree, however as far as I could tell, it wasn't actually getting used/compiled? To test this I added an #error "This file is not getting compiled" line to component.cc (and a few other files), and my fx build (on a configuration that uses Flutter) still ran successfully.

@liyuqian
Copy link
Contributor

@nathanrogersgoogle : Ah, I just learnt from Kaushik that we've changed the way of working with the locally built engine in fuchsia. Please check https://docs.google.com/document/d/1osKu_vDe7gAflDDQwjlf8OepViTtJodrCaPo3JXfXqY/edit#heading=h.xsio5oaxyhrv

@cbracken cbracken added the Work in progress (WIP) Not ready (yet) for review! label Nov 4, 2019
@cbracken
Copy link
Member

@nathanrogersgoogle are you still working on this?

@nathanrogersgoogle
Copy link
Contributor Author

Yes, I am still working on this. It's in my TODO list to try out the instructions at https://docs.google.com/document/d/1osKu_vDe7gAflDDQwjlf8OepViTtJodrCaPo3JXfXqY/edit#heading=h.xsio5oaxyhrv.

@nathanrogersgoogle
Copy link
Contributor Author

Status: Still looking to check this in, however I still haven't been able to get a build off. I'm attempting to follow instructions at https://docs.google.com/document/d/1XZMtJWiBnRCgf9cqdfiTHpyAiHFLVxeTUMke3zSFLag/edit?ts=5dd444cc, however am getting stuck on step 7 (both options there, including the link in the 2nd option, don't seem to run successfully for me).

@cbracken
Copy link
Member

cbracken commented Dec 9, 2019

/cc @iskakaushik

@iskakaushik iskakaushik self-requested a review December 9, 2019 18:50
@nathanrogersgoogle nathanrogersgoogle changed the title [WIP] Enable Skia tracing by default on Fuchsia [flutter_runner] Enable Skia tracing by default on Fuchsia Dec 11, 2019
@nathanrogersgoogle nathanrogersgoogle marked this pull request as ready for review December 11, 2019 04:39
@nathanrogersgoogle
Copy link
Contributor Author

This change is now ready for review.

Verified that category "skia" events are present when running something like fx traceutil record --categories=skia, and that the performance impact on Fuchsia' e2e performance tests is negligible.

iskakaushik
iskakaushik approved these changes Dec 11, 2019
View reviewed changes
Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

LGTM

liyuqian
liyuqian approved these changes Dec 11, 2019
View reviewed changes
Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

LGTM

@nathanrogersgoogle
[flutter_runner] Enable Skia tracing by default on Fuchsia
c6fb5e6 
Since Flutter tracing is wired up to Fuchsia system level tracing (and
that includes Skia tracing within Flutter), it makes more sense to
enable Skia tracing by default on Fuchsia, and to control Flutter Skia
tracing, rely on whether Fuchsia system tracing is enabled, in progress,
and contains the "skia" category.
@nathanrogersgoogle nathanrogersgoogle merged commit 1ce85be into flutter:master Dec 12, 2019
@nathanrogersgoogle nathanrogersgoogle deleted the enable-fuchsia-skia-tracing branch December 12, 2019 05:07
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 13, 2019
@engine-flutter-autoroll
1ce85be [flutter_runner] Enable Skia tracing by default on Fuchsia (f…
11bd8df 
…lutter/engine#13457)
This was referenced Dec 13, 2019
Closed
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 13, 2019
@engine-flutter-autoroll
1ce85be [flutter_runner] Enable Skia tracing by default on Fuchsia (f…
f39e6f2 
…lutter/engine#13457)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 13, 2019
@engine-flutter-autoroll
1ce85be [flutter_runner] Enable Skia tracing by default on Fuchsia (f…
ab3850f 
…lutter/engine#13457)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 13, 2019
Merged
iskakaushik pushed a commit to flutter/flutter that referenced this pull request Dec 13, 2019
@engine-flutter-autoroll @iskakaushik
Roll engine db60ebc..e0e0ac0 (23 commits) (#46933)
67adb89 
* fb9dfe0 [fuchsia] Move async_get_default_dispatcher include to the header (flutter/engine#14351)

* 3ebb7b4 Roll src/third_party/skia 75799967be60..3517aa7b14ad (3 commits) (flutter/engine#14345)

* 2713225 Remove duplicate and outdated src/third_party/dart/tools/sdks entry from DEPS. (flutter/engine#14340)

* 80d80ff Add ability to control dithering on Paint (flutter/engine#13868)

* 8595361 Conditionally use offscreen root surface only when needed

* 0a40f3d Assert that arc end caps on canvases with root surface transformations are drawn correctly. (flutter/engine#14359)

* d698d96 Fix missing timeline event of flutter engine's startup time (flutter/engine#14319)

* 9dc23b8 Fix missing API stream when record event in systrace (flutter/engine#14323)

* 9e4c6ad Fix CGMutablePathRef memory leaks when the path is invalid. (flutter/engine#14275)

* fc8cafb objcdoc fix for some ambiguity (flutter/engine#14367)

* 9bafb3c [tests] Use distinct begin and end times (flutter/engine#14361)

* 897ce34 Roll src/third_party/skia 3517aa7b14ad..826484f2631f (18 commits) (flutter/engine#14375)

* 1ce85be [flutter_runner] Enable Skia tracing by default on Fuchsia (flutter/engine#13457)

* a7b6ee5 Smart quote/dash configuration support in iOS (flutter/engine#13863)

* 48ba39c Roll fuchsia/sdk/core/mac-amd64 from otkJA... to SlgE8... (flutter/engine#14407)

* 0081e8c Remove unused _TypeNone enum field. (flutter/engine#14440)

* d8edfea Roll src/third_party/dart d9fa37e85d5c..45db29709547 (48 commits) (flutter/engine#14453)

* f650bca Refactoring text editing. Strategy pattern is used to handle different browser/operating system and a11y behavior. (flutter/engine#14131)

* 4275b49 Fix type in build_fuchsia_artifacts (flutter/engine#14452)

* 0c24f3d Roll src/third_party/skia 51b99659ed82..c514e7d9be6e (13 commits) (flutter/engine#14457)

* ffbe2a4 [testing] Move test vsync waiters to their own TUs (flutter/engine#14456)

* 181ad4e Use futures to images used for comparison with fixtures in embedder unit-tests. (flutter/engine#14465)

* e0e0ac0 [testing] Make vsync waiters pluggable in shell_unittests (flutter/engine#14463)
filmil pushed a commit to filmil/engine that referenced this pull request Mar 13, 2020
@nathanrogersgoogle @filmil
[flutter_runner] Enable Skia tracing by default on Fuchsia (flutter#1…
2aa4323 
…3457)

Since Flutter tracing is wired up to Fuchsia system level tracing (and
that includes Skia tracing within Flutter), it makes more sense to
enable Skia tracing by default on Fuchsia, and to control Flutter Skia
tracing, rely on whether Fuchsia system tracing is enabled, in progress,
and contains the "skia" category.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iskakaushik iskakaushik iskakaushik approved these changes

@liyuqian liyuqian liyuqian approved these changes

Assignees
No one assigned
Labels
cla: yes Work in progress (WIP) Not ready (yet) for review!
Projects
None yet
Milestone
No milestone
5 participants
@nathanrogersgoogle @liyuqian @cbracken @iskakaushik @googlebot