fire service protocol extension events for frames #10966

devoncarew · 2017-06-26T01:10:30Z

We're currently recording frame info to the VM timeline; this PR also fires service protocol extension events for flutter frames. The event includes the frameNumber, the startTime (in micros, from app start), and elapsed time, in micros. Debugger clients can opt-into this event via streamListen('Extension').

This info will allow clients to show a bit more awareness of real-time frame times.

@Hixie

Hixie · 2017-06-26T18:20:07Z

packages/flutter/lib/src/scheduler/binding.dart

  Duration _currentFrameTimeStamp;

  int _debugFrameNumber = 0;
+  Stopwatch _frameStopwatch;


if this is debug-only, please name it _debugFrameStopwatch

Hixie · 2017-06-26T18:21:54Z

Seems a bit dubious to be recording this information in debug mode, since we don't make particular efforts to get good performance in debug builds. Ideally this would be profile-mode-only.

devoncarew · 2017-06-26T18:30:31Z

Ideally this would be profile-mode-only.

Ah, yeah, we would want the events when in profile mode. Is there a way to recognize that in the framework? (I realize that the asserts will all be stripped out at that point)

Hixie · 2017-06-26T19:40:04Z

I think that's a Dart-level problem. Ideally I'd like a way to have profile-mode-only code stripped out of the binary in release builds.

devoncarew · 2017-06-26T23:50:00Z

We have a way today to recognize debug mode (via assert()s). I'm guessing we don't have a way to differentiate between profile and release modes? Ideally I think I'd want the frame events available in debug and profile modes (profile for accurate, realtime frame info, and debug mode because the info is still interesting, and it would allow tools to give a more live sense of the running app.

Hixie · 2017-06-29T17:34:17Z

I suggest we create a function in the foundation layer called profile that takes a VoidCallback and that only calls its argument if we're in profile mode. Then, we should file a bug on the VM asking for a way to guarantee (with a test) that this function is only included in profile mode, and that it is entirely omitted (including even the jump into the function) in release builds.

devoncarew · 2017-06-29T17:39:23Z

sgtm; I'm in the process of re-working this given the const bool.fromEnvironment("dart.vm.product") test. An initial version of profile() could be based on that.

…e (or debug) modes

devoncarew · 2017-06-29T18:55:49Z

@Hixie, re-worked, but I could use some guidance about where profile() should live.

devoncarew · 2017-06-30T00:15:16Z

(I added profile() to foundation/profile.dart)

Hixie · 2017-07-19T00:20:17Z

packages/flutter/lib/src/foundation/profile.dart

+/// When running in profile mode (or debug mode), invoke the given function.
+///
+/// In release mode, the function is not invoked. In the future, we'd want the
+/// given closure - and the call to [profile] - to be tree-shaken out.


The "In the future" sentence should probably just be a TODO() rather than part of the documentation.

Hixie · 2017-07-19T00:24:18Z

This looks fine but needs a test for the Flutter.Frame logic (verifying that the data is being sent in the expected format, for example, so that nobody accidentally breaks the protocol during a global search-and-replace of the string "startTime" or some such).

devoncarew · 2017-07-19T01:50:27Z

This looks fine but needs a test for the Flutter.Frame logic

👍, will add a test

devoncarew · 2017-07-19T01:58:33Z

Just for context, here is the real-time display I'm playing with in IntelliJ:

sethladd · 2017-07-19T21:31:26Z

cc @DanTup for ideas for VS Code :)

@devoncarew this looks great, thanks!

devoncarew · 2017-07-19T22:10:29Z

@Hixie, I did refactor this a bit for better testability; we now have an event stream for the frame information, and that gets copied into service protocol events. It's not exactly the test you mentioned above, but does test that we're creating frame info events w/ the expected info.

Stream<FrameInfo> get onFrameInfo;

Hixie · 2017-07-20T00:25:56Z

packages/flutter/lib/src/scheduler/binding.dart

  return true;
 }
+
+/// Performance information for a Flutter frame.


we need much more information here, such as, how to get an instance of such a class, what information it stores, what modes it would be available in, the protocol by which it's sent over to IDEs or pointers to documentation on that protocol, etc.

Hixie · 2017-07-20T00:26:03Z

packages/flutter/lib/src/scheduler/binding.dart

+
+  /// The frame number.
+  final int number;
+  /// The start time in microseconds.


blank lines between members

Hixie · 2017-07-20T00:26:14Z

packages/flutter/lib/src/scheduler/binding.dart

+class FrameInfo {
+  FrameInfo(this.number, this.startTime, this.elapsed);
+
+  /// The frame number.


this documentation doesn't comply to our style guide.

Hixie · 2017-07-20T00:27:00Z

packages/flutter/lib/src/scheduler/binding.dart

+
+  /// The frame number.
+  final int number;
+  /// The start time in microseconds.


"start time in microseconds" only raises more questions! relative to what? is it monotonic? is it affected by timeDilation? etc etc etc

Hixie · 2017-07-20T00:27:33Z

packages/flutter/lib/src/scheduler/binding.dart

+  final int elapsed;
+
+  @override
+  String toString() => 'frame $number, elapsed time: $elapsed micros';


rather than "micros" let's just say "µs".

Hixie · 2017-07-20T00:27:55Z

packages/flutter/lib/src/scheduler/binding.dart

-  int _debugFrameNumber = 0;
+  int _profileFrameNumber = 0;
+  Stopwatch _profileFrameStopwatch = new Stopwatch();
+  StreamController<FrameInfo> _frameInfoController = new StreamController<FrameInfo>.broadcast();


let's not initialise this in release mode...

also presumably it should be final?

Hixie · 2017-07-20T00:28:55Z

packages/flutter/lib/src/scheduler/binding.dart

+  /// These events are created after a frame is rendered and contain performance related
+  /// information. Events are only fired when the app is running in debug or profile mode;
+  /// no frame information events are fired in release mode.
+  Stream<FrameInfo> get onFrameInfo => _frameInfoController.stream;


this member doesn't comply with our style guide's naming convention.

Hixie · 2017-07-20T00:29:24Z

i'm not entirely convinced that exposing this stream to the app makes much sense... what's the use case other than tests?

devoncarew · 2017-07-20T14:27:38Z

i'm not entirely convinced that exposing this stream to the app makes much sense... what's the use case other than tests?

I don't have any other use case in mind than for tests. It looks like we do expose some other timing related information in SchedulerBinding. I'm also happy to try and reduce the visibility of the API to get at the stream of event info.

Hixie · 2017-07-20T15:26:19Z

I'm just dubious about instantiating a whole Stream object and so forth in applications, just for testing purposes. That's a very high cost for something that happens very rarely.

Hixie · 2017-07-20T15:27:39Z

Maybe you could collect the data then pass it to a protected method that calls developer.whatever, and in the test just subclass the binding and override that method?

The real thing that needs testing though is the VM service protocol and none of these tests are going to do that...

devoncarew · 2017-07-20T15:47:58Z

Maybe you could collect the data then pass it to a protected method that calls developer.whatever, and in the test just subclass the binding and override that method?

I'll do some noodling, and either rework for visibility, or move to another type of test (a devicelab test) which tests at a different level.

The real thing that needs testing though is the VM service protocol and none of these tests are going to do that...

I agree it would be good to test at that level - not sure that we currently are for our other service extension work.

Hixie · 2017-07-20T16:15:48Z

not sure that we currently are for our other service extension work.

Someone has to be the first. :-)

For example, until yesterday, we didn't have any test of flutter run that actually tested that pressing keys did anything. So when testing my new "show performance overlay" feature, rather than testing it the way we test the other features, I created a new kind of test that runs flutter run, and writes to its stdin stream, then runs flutter drive tests to see if it's having any effect. Bam, new kind of test. :-)

DanTup · 2017-07-20T19:00:39Z

@sethladd I don't believe Code has any way for us to render arbitrary content like this :(

Also; we don't currently support profiling mode (I don't really know much about this) which I figure is where this makes sense. I've raised Dart-Code/Dart-Code#373 to have this in the list though :-)

devoncarew · 2017-07-27T19:20:53Z

@Hixie - circling back to this one - I updated it to add a devicelab test, and removed the public API around listening for frame events (we're back to only firing them on the service protocol).

Hixie · 2017-07-27T20:45:41Z

dev/devicelab/bin/tasks/service_extensions_test.dart

+      expect(event.data['startTime'] is int);
+      expect(event.data['startTime'] >= 0);
+      expect(event.data['elapsed'] is int);
+      expect(event.data['elapsed'] >= 0);


this is fantastic. thank you!

Hixie · 2017-07-27T20:47:10Z

This is wonderful.

devoncarew · 2017-07-27T22:34:50Z

bombs away

devoncarew · 2017-07-27T22:53:57Z

Looks like this is turning the devicelab bots red (https://flutter-dashboard.appspot.com/api/get-log?ownerKey=ahNzfmZsdXR0ZXItZGFzaGJvYXJkclgLEglDaGVja2xpc3QiOGZsdXR0ZXIvZmx1dHRlci80YjRjYWJiNzYxZWMxY2I3ZmI1ZjJlOGVjNmU4NTBkZTI5YTAzMDU2DAsSBFRhc2sYgICAgICAyAsM). I'm not sure why, so may revert in the meantime,

)" This reverts commit 4b4cabb.

…11430) This reverts commit 4b4cabb.

devoncarew added 2 commits June 25, 2017 18:04

fire service protocol extension events for frames

3194de8

start time in micros

bfb03fc

googlebot added the cla: yes label Jun 26, 2017

Hixie reviewed Jun 26, 2017

View reviewed changes

devoncarew added 2 commits June 29, 2017 10:45

Merge branch 'master' into fire_events_for_frames

d930c1d

introduce a profile() function; only send frame events when in profil…

caea192

…e (or debug) modes

moved the profile() function to foundation/profile.dart

9ce4358

Hixie reviewed Jul 19, 2017

View reviewed changes

devoncarew added 3 commits July 19, 2017 10:38

Merge branch 'master' into fire_events_for_frames

247fd9f

refactor to make the change more testable; test the change

de521a0

Merge branch 'master' into fire_events_for_frames

f9da020

Hixie reviewed Jul 20, 2017

View reviewed changes

fire service protocol events by listening to onFrameInfo

4e8c26b

DanTup mentioned this pull request Jul 20, 2017

Support Flutter profiling/release mode Dart-Code/Dart-Code#373

Closed

devoncarew added 4 commits July 27, 2017 10:20

Merge branch 'master' into fire_events_for_frames

3f3af5d

remove the frame event stream; add a devicelab test

96f2bc9

remove a todo

c7b4e69

final

75e8758

Hixie reviewed Jul 27, 2017

View reviewed changes

devoncarew merged commit 4b4cabb into flutter:master Jul 27, 2017

devoncarew added a commit to devoncarew/flutter that referenced this pull request Jul 27, 2017

Revert "fire service protocol extension events for frames (flutter#10966

51c8cef

)" This reverts commit 4b4cabb.

devoncarew added a commit that referenced this pull request Jul 27, 2017

Revert "fire service protocol extension events for frames (#10966)" (#…

ba5bb57

…11430) This reverts commit 4b4cabb.

This was referenced Jul 28, 2017

add a profile() method #11443

Merged

refactor commands_test to choose a free port for the service protocol #11446

Merged

devoncarew mentioned this pull request Aug 9, 2017

fire service protocol events for frames #11565

Merged

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

fire service protocol extension events for frames #10966

fire service protocol extension events for frames #10966

Uh oh!

Conversation

devoncarew commented Jun 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jun 26, 2017

Uh oh!

devoncarew commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hixie commented Jun 26, 2017

Uh oh!

devoncarew commented Jun 26, 2017

Uh oh!

Hixie commented Jun 29, 2017

Uh oh!

devoncarew commented Jun 29, 2017

Uh oh!

devoncarew commented Jun 29, 2017

Uh oh!

devoncarew commented Jun 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jul 19, 2017

Uh oh!

devoncarew commented Jul 19, 2017

Uh oh!

devoncarew commented Jul 19, 2017

Uh oh!

sethladd commented Jul 19, 2017

Uh oh!

devoncarew commented Jul 19, 2017

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!

Hixie commented Jul 20, 2017

Uh oh!

devoncarew commented Jul 20, 2017

Uh oh!

Hixie commented Jul 20, 2017

Uh oh!

Hixie commented Jul 20, 2017

Uh oh!

devoncarew commented Jul 20, 2017

Uh oh!

Hixie commented Jul 20, 2017

Uh oh!

DanTup commented Jul 20, 2017

Uh oh!

devoncarew commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jul 27, 2017

Uh oh!

devoncarew commented Jul 27, 2017

Uh oh!

devoncarew commented Jul 27, 2017

Uh oh!

Reviewers

Assignees

Labels

devoncarew commented Jun 26, 2017 •

edited

Loading