Image tracing by dnfield · Pull Request #50648 · flutter/flutter

dnfield · 2020-02-12T17:58:16Z

Description

Add tracing to image cache calls outside of release mode. Looks like this:

And can be easily lined up with engine work to see what's going on. The small line dropping off of listener is to a cleanup routine that runs at the end of listening, which is too small to be visible at the zoom level of the screenshot (but is there!) :).

/cc @liyuqian @chinmaygarde @alml @ignatz @goderbauer @gaaclarke

Tests

I added the following tests:

I'm trying to figure out the right way to test this. I think we'll need to update a driver test and check that this data is in there to start, but I'm open to suggestions.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

fluttergithubbot · 2020-02-12T17:58:18Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

dnfield · 2020-02-12T21:41:57Z

I'm working on tests for this, need to land a couple other things to enable it first - #50663 and #50666

dnfield · 2020-02-12T22:58:40Z

I tried to keep the package updates out of this but two more snuck in. Oh well.

packages/flutter/pubspec.yaml

packages/flutter_tools/pubspec.yaml

jonahwilliams · 2020-02-12T23:01:42Z

🤷‍♂️

You could try pinning it in update_packages.dart

bkonyi

Timeline code and test LGTM

bkonyi · 2020-02-12T23:05:24Z

packages/flutter/test/tracing/image_cache_tracing_test.dart

+    List<TimelineEvent> events, List<Map<String, dynamic>> expected) {
+  for (final TimelineEvent event in events) {
+    for (int index = 0; index < expected.length; index += 1) {
+      if (expected[index]['name'] == event.json['name']) {


This would be cleaner if you did:

for (final e in expected) { ... if (_mapsEqual(expectedArgs, args)) { expected.remove(e); } }

_mapsEqual assumes it's doing a flat map though, and there are no nested maps. I guess I could update it to check for that...

I'm trying to short circuit the more expensive recursion this way though by keying on the name. It's faster to check that one property than to recurse the entire structure, which could become large as we add to these tests.

bkonyi · 2020-02-12T23:07:47Z

packages/flutter/lib/src/painting/image_cache.dart

      return;
+    TimelineTask timelineTask;
+    if (!kReleaseMode) {
+       timelineTask = TimelineTask()..start('ImageCache.setMaximumSize', arguments: <String, dynamic>{'value': value});


Nit: You might want to consider breaking this into multiple lines (below as well)

dnfield · 2020-02-12T23:35:12Z

This test either has to be rewritten (the dwds pinned version of vm_service is broken for what the test uses), or we have to get dwds to bump vmservice for this.

I'm separately going to open an internal CL to open up visibility for flutter to use this package.

dnfield · 2020-02-13T01:02:47Z

I rewrote the test to not depend on package:vm_service. Filed dart-lang/webdev#899 to track getting that bumped in dwds and added a TODO.

dev/bots/test.dart

goderbauer

LGTM

goderbauer · 2020-02-13T18:13:56Z

dev/tracing_tests/README.md

@@ -0,0 +1,7 @@
+# Tracing tests
+
+The tests in this folder must be run with `flutter test --enable-vmservice`,


Any chance you can check for this condition from within the test and just print this message if the test was run incorrectly?

(Readmes usually get ignored..)

Never mind, I see below that you already do this: 🌟

goderbauer · 2020-02-13T18:14:38Z

dev/tracing_tests/test/image_cache_tracing_test.dart

+import 'dart:isolate' as isolate;
+
+import 'package:flutter/painting.dart';
+


nit: I believe this blank line shouldn't be here?

jonahwilliams · 2020-02-13T21:00:01Z

packages/flutter/test/tracing/README.md

@@ -0,0 +1,7 @@
+# Tracing tests


Should this be removed?

The whole file or this line?

I think most of the folders in dev/ have a README.md - the goal of this one is to make it clear what kind of tests belong in here, and what's special about running them.

This isn't in dev though, this is in flutter/test

Ah! Thanks, good catch. Removed.

jonahwilliams

LGTM

dnfield added 3 commits February 11, 2020 16:03

start

cfefb45

..

4733e3f

Trace image cache calls

bded44a

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 12, 2020

googlebot added the cla: yes label Feb 12, 2020

test

660a47f

dnfield mentioned this pull request Feb 12, 2020

add an --enable-observatory flag #50663

Merged

dnfield added 2 commits February 12, 2020 14:53

Merge remote-tracking branch 'upstream/master' into image_tracing

13972fe

test, packages, merge

5c7e9c2

dnfield requested review from bkonyi, gaaclarke, goderbauer and jonahwilliams February 12, 2020 22:58

dnfield commented Feb 12, 2020

View reviewed changes

packages/flutter/pubspec.yaml Outdated Show resolved Hide resolved

dnfield commented Feb 12, 2020

View reviewed changes

packages/flutter_tools/pubspec.yaml Outdated Show resolved Hide resolved

bkonyi approved these changes Feb 12, 2020

View reviewed changes

fix packages

6148146

dnfield force-pushed the image_tracing branch from 2569ea0 to 6148146 Compare February 12, 2020 23:21

make it work

e84f8df

newline

d42d465

dnfield commented Feb 13, 2020

View reviewed changes

dev/bots/test.dart Outdated Show resolved Hide resolved

fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Feb 13, 2020

dnfield requested a review from bkonyi February 13, 2020 01:06

simplify test

4443c4d

dnfield added 4 commits February 12, 2020 17:28

fix re-entrancy

f7fc674

fix bots

0efc96b

move to separate location

9e37ee9

fix test for new location...

c708be9

dnfield force-pushed the image_tracing branch from 56766b8 to c708be9 Compare February 13, 2020 01:58

typo

b3dbbc0

dnfield mentioned this pull request Feb 13, 2020

Live image cache #50318

Merged

13 tasks

goderbauer reviewed Feb 13, 2020

View reviewed changes

Update image_cache_tracing_test.dart

b9f0f32

dnfield added the waiting for tree to go green label Feb 13, 2020

jonahwilliams reviewed Feb 13, 2020

View reviewed changes

oops

dae26f4

jonahwilliams approved these changes Feb 13, 2020

View reviewed changes

dnfield merged commit 766bd70 into flutter:master Feb 13, 2020

dnfield deleted the image_tracing branch February 13, 2020 22:04

github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021

		@@ -0,0 +1,7 @@
		# Tracing tests

		The tests in this folder must be run with `flutter test --enable-vmservice`,

		import 'dart:isolate' as isolate;

		import 'package:flutter/painting.dart';

Conversation

dnfield commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Tests

Checklist

Breaking Change

Uh oh!

fluttergithubbot commented Feb 12, 2020

Uh oh!

dnfield commented Feb 12, 2020

Uh oh!

dnfield commented Feb 12, 2020

Uh oh!

Uh oh!

Uh oh!

jonahwilliams commented Feb 12, 2020

Uh oh!

bkonyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Feb 12, 2020

Uh oh!

dnfield commented Feb 13, 2020

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dnfield commented Feb 12, 2020 •

edited

Loading