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

Conversation

@GaryQian
Copy link
Contributor

@GaryQian GaryQian commented Sep 11, 2018

Cache a SkSurface with previously drawn shapes so that we do not need to draw them again in future frames.

On Nexus 5X test device, old render time for just the overlay was 1.3ms-3.0ms and this version improves to 0.9ms-1.3ms running flutter gallery in profile mode.

@GaryQian GaryQian requested review from abarth, cbracken, chinmaygarde and liyuqian and removed request for abarth September 11, 2018 18:36
@GaryQian GaryQian changed the title Improve performance of performance overlay Improve performance of performance overlay by caching. Sep 11, 2018
@GaryQian
Copy link
Contributor Author

GaryQian commented Sep 11, 2018

So there is a slight difference in the horizontal black marker bar's thickness. In this version it looks like it is a pixel or two thicker, but otherwise, it should replicate the old version's functionality.

Perf numbers from a Moto G4:
old: 3-5ms
new: 1.4ms

@GaryQian
Copy link
Contributor Author

Fixes flutter/flutter#21087

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 with nits.

const fml::TimeDelta delta = fml::TimeDelta::Zero();
laps_.resize(kMaxSamples, delta);
cache_dirty_ = true;
prev_drawn_sample_ = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

num_prev_drawn_sample_ might be more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually meant to be the index of the previously drawn sample, not a count. This is so that if the overlay is turned off and then on again, the Stopwatch knows where to erase the red/green vertical bar previously drawn. prev_drawn_sample_index_ is probably a more fitting name.

const SkScalar right = x + width;

// Scale the graph to show frame times up to those that are 3 times the frame
// time.
Copy link
Contributor

Choose a reason for hiding this comment

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

This deleted comment seems to be still useful.

const SkScalar y = 0;
const SkScalar width = rect.width();
const SkScalar height = rect.height();
const SkScalar bottom = y + height;
Copy link
Contributor

Choose a reason for hiding this comment

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

Now bottom == height and right == width. Maybe we can just use width/height?

const SkScalar y = 0;
const SkScalar width = rect.width();
const SkScalar height = rect.height();
const SkScalar bottom = y + height;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: width/height is always equal to right/bottom.

@GaryQian GaryQian merged commit 838eb3d into flutter:master Sep 11, 2018
@GaryQian GaryQian deleted the perfoverlay branch September 11, 2018 23:13
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2018
flutter/engine@9a173a8...838eb3d

git log 9a173a8..838eb3d --no-merges --oneline
838eb3d Improve performance of performance overlay by caching. (flutter/engine#6225)
7ac3345 Remove root_surface_transformation from PaintContext (flutter/engine#6213)
565a194 Flutter roll for Dart. (flutter/engine#6227)

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, who should
be CC'd on the roll, and stop the roller if necessary.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2018
flutter/engine@9a173a8...4057327

git log 9a173a8..4057327 --no-merges --oneline
4057327 Roll src/third_party/skia bd6595544171..1b5ece0f06f4 (22 commits) (flutter/engine#6229)
838eb3d Improve performance of performance overlay by caching. (flutter/engine#6225)
7ac3345 Remove root_surface_transformation from PaintContext (flutter/engine#6213)
565a194 Flutter roll for Dart. (flutter/engine#6227)

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, who should
be CC'd on the roll, and stop the roller if necessary.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2018
flutter/engine@e65beb8...0ded891

git log e65beb8..0ded891 --no-merges --oneline
0ded891 Roll src/third_party/skia 1b5ece0f06f4..e70aed7066c6 (1 commits) (flutter/engine#6231)
4057327 Roll src/third_party/skia bd6595544171..1b5ece0f06f4 (22 commits) (flutter/engine#6229)
838eb3d Improve performance of performance overlay by caching. (flutter/engine#6225)
7ac3345 Remove root_surface_transformation from PaintContext (flutter/engine#6213)
565a194 Flutter roll for Dart. (flutter/engine#6227)
9a173a8 Roll buldroot to a11c4fd (flutter/engine#6224)
51b26f6 Revert "Roll buildroot to eba79bb (flutter#6215)" (flutter/engine#6223)
7be4462 Roll src/third_party/skia 2bf7a7bcc67f..bd6595544171 (4 commits) (flutter/engine#6222)
54fe12b Roll src/third_party/skia 82bf31003c66..2bf7a7bcc67f (1 commits) (flutter/engine#6221)
35ddf87 Roll buildroot to eba79bb (flutter/engine#6215)
6ed00a8 Roll src/third_party/skia 5518e65d90d7..82bf31003c66 (1 commits) (flutter/engine#6220)
8d9e20c Roll src/third_party/skia 3c4d533d8ebd..5518e65d90d7 (1 commits) (flutter/engine#6219)
8f39cac Roll src/third_party/skia 7891994e89a3..3c4d533d8ebd (1 commits) (flutter/engine#6218)
e3133e0 Reapply "Some cleanups enabled by removing support for Dart 1." (flutter/engine#6216)
1fb01f3 Roll src/third_party/skia a2bc1ca21bbc..7891994e89a3 (9 commits) (flutter/engine#6217)
f19ee56 Roll freetype2 to 6581fd3e9c8645f01c0d51e4f53893f5391f2bf3 (flutter/engine#6214)

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, who should
be CC'd on the roll, and stop the roller if necessary.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2018
flutter/engine@e65beb8...6f459e2

git log e65beb8..6f459e2 --no-merges --oneline
6f459e2 Revert "Reapply "Some cleanups enabled by removing support for Dart 1" (flutter#6216)" (flutter/engine#6232)
0ded891 Roll src/third_party/skia 1b5ece0f06f4..e70aed7066c6 (1 commits) (flutter/engine#6231)
4057327 Roll src/third_party/skia bd6595544171..1b5ece0f06f4 (22 commits) (flutter/engine#6229)
838eb3d Improve performance of performance overlay by caching. (flutter/engine#6225)
7ac3345 Remove root_surface_transformation from PaintContext (flutter/engine#6213)
565a194 Flutter roll for Dart. (flutter/engine#6227)
9a173a8 Roll buldroot to a11c4fd (flutter/engine#6224)
51b26f6 Revert "Roll buildroot to eba79bb (flutter#6215)" (flutter/engine#6223)
7be4462 Roll src/third_party/skia 2bf7a7bcc67f..bd6595544171 (4 commits) (flutter/engine#6222)
54fe12b Roll src/third_party/skia 82bf31003c66..2bf7a7bcc67f (1 commits) (flutter/engine#6221)
35ddf87 Roll buildroot to eba79bb (flutter/engine#6215)
6ed00a8 Roll src/third_party/skia 5518e65d90d7..82bf31003c66 (1 commits) (flutter/engine#6220)
8d9e20c Roll src/third_party/skia 3c4d533d8ebd..5518e65d90d7 (1 commits) (flutter/engine#6219)
8f39cac Roll src/third_party/skia 7891994e89a3..3c4d533d8ebd (1 commits) (flutter/engine#6218)
e3133e0 Reapply "Some cleanups enabled by removing support for Dart 1." (flutter/engine#6216)
1fb01f3 Roll src/third_party/skia a2bc1ca21bbc..7891994e89a3 (9 commits) (flutter/engine#6217)
f19ee56 Roll freetype2 to 6581fd3e9c8645f01c0d51e4f53893f5391f2bf3 (flutter/engine#6214)

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, who should
be CC'd on the roll, and stop the roller if necessary.
amirh pushed a commit to amirh/engine that referenced this pull request Sep 21, 2018
Cache a SkSurface with previously drawn shapes so that we do not need to draw them again in future frames.

On Nexus 5X test device, old render time for just the overlay was 1.3ms-3.0ms and this version improves to 0.9ms-1.3ms running flutter gallery in profile mode.
amirh pushed a commit to amirh/engine that referenced this pull request Sep 21, 2018
Cache a SkSurface with previously drawn shapes so that we do not need to draw them again in future frames.

On Nexus 5X test device, old render time for just the overlay was 1.3ms-3.0ms and this version improves to 0.9ms-1.3ms running flutter gallery in profile mode.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants