Skip to content

Conversation

@blasten
Copy link

@blasten blasten commented Mar 17, 2020

Fixes #52684

This PR adds a layout to the complex_layout app, which renders a few dummy platform views and an animated overlay on top of each platform view. The driver test scrolls the list and gathers information such as the 90th percentile frame build time in millisec.

@fluttergithubbot fluttergithubbot added c: contributor-productivity Team-specific productivity, code health, technical debt. work in progress; do not review labels Mar 17, 2020
@blasten blasten changed the title Implement DummyPlatformView on iOS Add benchmark for platform views Mar 17, 2020
@blasten blasten marked this pull request as ready for review March 17, 2020 22:54
@blasten blasten requested a review from cyanglaz March 17, 2020 22:56
@blasten blasten requested a review from dnfield March 17, 2020 23:11

<application android:name="io.flutter.app.FlutterApplication" android:label="complex_layout" android:icon="@mipmap/ic_launcher">
<activity android:name="io.flutter.app.FlutterActivity"
<activity android:name=".DummyPlatformViewActivity"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should avoid touching this file right now - it would probably be better to just create a new benchmark for this rather than touching the existing one, since this is used by several other benchmarks.

I'm particularly worried about what happens if this puts a devicelab device into a bad state right now.

/cc @godofredoc

Comment on lines 44 to 45
<key>io.flutter.embedded_views_preview</key>
<true/>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be done in a different application. This will impact other benchmarks besides this one, since it changes the threading model used by the application on iOS.

Copy link
Author

Choose a reason for hiding this comment

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

This will become the default though. cc @cyanglaz what is the status of that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Still have several PRs waiting on different people to reviews. It might take some time as people have limited availability working from home. If you want to land this benchmark soon, we should create a different App.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

Code overall looks good, I'm worried about impact application changes might have on other benchmarks.

@blasten
Copy link
Author

blasten commented Mar 18, 2020

I created a separate app in the meanwhile. Adding a new app to the flutter repo involves a non-trivial diff, so I tried to reuse as much of the existing one. Once thread merging becomes the new default, complex_layout and platform_views_layout can be combined. PTAL

@blasten blasten requested a review from dnfield March 18, 2020 00:35
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!
As long as ["linux/android"] and ["mac/ios"] always running on the same android and ios devices.

@blasten blasten merged commit 1f17318 into flutter:master Mar 18, 2020
@blasten blasten deleted the pv_benchmark branch March 18, 2020 17:55
blasten pushed a commit that referenced this pull request Mar 18, 2020
blasten pushed a commit that referenced this pull request Mar 18, 2020
@liyuqian liyuqian added perf: speed Performance issues related to (mostly rendering) speed c: performance Relates to speed or footprint issues (see "perf:" labels) labels Mar 28, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. c: performance Relates to speed or footprint issues (see "perf:" labels) perf: speed Performance issues related to (mostly rendering) speed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add benchmark for testing performance of platform views

6 participants