Make Flutter Frame timeline live and migrate to the FrameTiming API #3168

kenzieschmoll · 2021-06-28T21:50:47Z

Tests are WIP - will work on those while this change is under review.

Apologies for the messy commit history - avoiding a nasty rebase.

packages/devtools_app/lib/src/common_widgets.dart

packages/devtools_app/lib/src/performance/performance_controller.dart

jacob314 · 2021-06-29T17:40:12Z

packages/devtools_app/lib/src/performance/performance_controller.dart

@@ -356,14 +517,37 @@ class PerformanceController

  void addTimelineEvent(TimelineEvent event) {
    data.addTimelineEvent(event);
+    _timelineEvents.value.add(event);


Adding elements to a ValueListenable without notifying isn't safe. Can you use ListValueNotifier or change the _timelineEvents so they are not a ValueListenable?

turns out we weren't even using the _timelineEvents notifier in the UI. removed it completely.

jacob314 · 2021-06-29T17:44:44Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+  /// These timeline events are keyed by the [FlutterFrame] ID specified in the
+  /// event arguments, which matches the ID for the corresponding
+  /// [FlutterFrame].
+  final unassignedFlutterFrameEvents = <int, List<TimelineEvent>>{};


Maybe replace List with a class with a clearer api.
Having the indexes in the List happen to be FlutterFrame ids is a little surprising.
class could be UnasignedEvents or similar.

added a class FrameTimelineEventData to store the timeline event data for a flutter frame.

jacob314 · 2021-06-29T17:46:03Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+      autoDispose(serviceManager.service.onTimelineEvent.listen((event) {
+        final eventBatch = <TraceEventWrapper>[];
+        if (event.json['kind'] == 'TimelineEvents') {
+          final List<dynamic> traceEvents = event.json['timelineEvents']


why is this a List?

This is how events are streamed over service.onTimelineEvent

jacob314 · 2021-06-29T17:46:17Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+                ),
+              )
+              .toList();
+          final List<TraceEventWrapper> wrappedTraceEvents =


avoid types on LHS when not needed.

jacob314 · 2021-06-29T17:50:51Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+  }
+
+  FutureOr<void> processAvailableEvents() async {
+    _processing.value = true;


nit: add
assert(!_processing.value) at the start of the method to make sure we don't get into a state where we have two pending processing requests.

jacob314 · 2021-06-29T17:52:43Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+      // Only try to pull timeline events for frames that are after the first
+      // well formed frame. Timeline events that occurred before this frame will
+      // have already fallen out of the buffer.
+      await processAvailableEvents();


Are we concerned this will hide timeline events on startup that occur before the first flutter frame (assuming such events can exist)?
We could always add an artificial "before startup" frame to disambiguate this case from the case you are probably trying to avoid of incomplete frames due to full buffers.

The timeline events that occur before the first frame will still be present in the timeline flame chart. This check is to prevent us from trying to process new events for frames that we know occurred before the first frame-related timeline events that we have data for. This code is only triggered when a frame is selected, so it is not responsible for the initial load of timeline events.

jacob314 · 2021-06-29T17:53:55Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+        final unassignedEventsForFrame =
+            unassignedFlutterFrameEvents.putIfAbsent(
+          frameNumber,
+          () => List.generate(2, (_) => null),


This is a case where having an UnasignedEvents class would make it clearer.

added a class FrameTimelineEventData to store the timeline event data for a flutter frame.

jacob314 · 2021-06-29T17:54:55Z

packages/devtools_app/lib/src/performance/performance_controller.dart

      }
    }
  }

+  void toggleRecordingFrames(bool recording) {
+    _recordingFrames.value = recording;
+    if (_recordingFrames.value) {


tiny nit: maybe condition on recording instead of the value of _recordingFrames.value

jacob314 · 2021-06-29T17:57:24Z

packages/devtools_app/lib/src/performance/performance_model.dart

@@ -284,7 +292,15 @@ class OfflinePerformanceData extends PerformanceData {
    final CpuProfileData cpuProfileData =
        cpuProfileJson.isNotEmpty ? CpuProfileData.parse(cpuProfileJson) : null;

-    final String selectedFrameId = json[PerformanceData.selectedFrameIdKey];
+    final int selectedFrameId = json[PerformanceData.selectedFrameIdKey];
+


nit: avoid dynamic where possible preferring Object so we get better static errors.

jacob314 · 2021-06-29T20:40:03Z

packages/devtools_app/lib/src/performance/timeline_flame_chart.dart

+        Notifications.of(context)
+            .push('No timeline events available for the selected frame');
+        return;
+      } else if (_selectedFrame.uiEventFlow == null) {


should these checks be added to a helper on _slectedFrame object? get the current time and event?

added helpers as extension methods since these really have more to do with the flame chart than the FlutterFrame class

jacob314 · 2021-06-29T20:41:50Z

packages/devtools_app/lib/src/performance/timeline_flame_chart.dart

+      } else if (_selectedFrame.uiEventFlow == null) {
+        time = _selectedFrame.rasterEventFlow.time;
+        event = _selectedFrame.rasterEventFlow;
+      } else if (_selectedFrame.rasterEventFlow == null) {


Also document why these checks are the way they are. I would have expected the check to be that
uiEventFlow != null rather than checking that rasterEventFlow == null as the current way would lead to a NPE if both were null for a frame but perhaps that isn't possible.

if both were null for a frame, we would have already returned above. I'll switch to use the != null check though as that may make it more clear.

kenzieschmoll

addressed review comments - working on adding tests

kenzieschmoll · 2021-06-29T20:51:05Z

packages/devtools_app/lib/src/performance/performance_model.dart

@@ -284,7 +292,15 @@ class OfflinePerformanceData extends PerformanceData {
    final CpuProfileData cpuProfileData =
        cpuProfileJson.isNotEmpty ? CpuProfileData.parse(cpuProfileJson) : null;

-    final String selectedFrameId = json[PerformanceData.selectedFrameIdKey];
+    final int selectedFrameId = json[PerformanceData.selectedFrameIdKey];
+


kenzieschmoll · 2021-06-29T20:58:20Z

packages/devtools_app/lib/src/performance/timeline_flame_chart.dart

+      } else if (_selectedFrame.uiEventFlow == null) {
+        time = _selectedFrame.rasterEventFlow.time;
+        event = _selectedFrame.rasterEventFlow;
+      } else if (_selectedFrame.rasterEventFlow == null) {


if both were null for a frame, we would have already returned above. I'll switch to use the != null check though as that may make it more clear.

kenzieschmoll · 2021-06-29T21:01:40Z

packages/devtools_app/lib/src/performance/timeline_flame_chart.dart

+        Notifications.of(context)
+            .push('No timeline events available for the selected frame');
+        return;
+      } else if (_selectedFrame.uiEventFlow == null) {


added helpers as extension methods since these really have more to do with the flame chart than the FlutterFrame class

kenzieschmoll · 2021-06-29T21:50:30Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+        final unassignedEventsForFrame =
+            unassignedFlutterFrameEvents.putIfAbsent(
+          frameNumber,
+          () => List.generate(2, (_) => null),


added a class FrameTimelineEventData to store the timeline event data for a flutter frame.

kenzieschmoll · 2021-06-29T21:50:50Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+  /// These timeline events are keyed by the [FlutterFrame] ID specified in the
+  /// event arguments, which matches the ID for the corresponding
+  /// [FlutterFrame].
+  final unassignedFlutterFrameEvents = <int, List<TimelineEvent>>{};


added a class FrameTimelineEventData to store the timeline event data for a flutter frame.

kenzieschmoll · 2021-06-29T21:51:51Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+                ),
+              )
+              .toList();
+          final List<TraceEventWrapper> wrappedTraceEvents =


kenzieschmoll · 2021-06-29T21:53:30Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+  }
+
+  FutureOr<void> processAvailableEvents() async {
+    _processing.value = true;


kenzieschmoll · 2021-06-29T21:54:33Z

packages/devtools_app/lib/src/performance/performance_controller.dart

      }
    }
  }

+  void toggleRecordingFrames(bool recording) {
+    _recordingFrames.value = recording;
+    if (_recordingFrames.value) {


kenzieschmoll · 2021-06-29T22:35:38Z

packages/devtools_app/lib/src/performance/performance_controller.dart

@@ -356,14 +517,37 @@ class PerformanceController

  void addTimelineEvent(TimelineEvent event) {
    data.addTimelineEvent(event);
+    _timelineEvents.value.add(event);


turns out we weren't even using the _timelineEvents notifier in the UI. removed it completely.

kenzieschmoll · 2021-06-29T22:39:58Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+      // Only try to pull timeline events for frames that are after the first
+      // well formed frame. Timeline events that occurred before this frame will
+      // have already fallen out of the buffer.
+      await processAvailableEvents();


The timeline events that occur before the first frame will still be present in the timeline flame chart. This check is to prevent us from trying to process new events for frames that we know occurred before the first frame-related timeline events that we have data for. This code is only triggered when a frame is selected, so it is not responsible for the initial load of timeline events.

jacob314 · 2021-07-13T16:23:10Z

…e handling

…frametiming

jacob314 · 2021-07-16T19:08:40Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+  Future<void> _pullTraceEventsFromVmTimeline({
+    bool shouldPrimeThreadIds = false,
+  }) async {
+    final currentVmTime = await serviceManager.service.getVMTimelineMicros();


need to make sure we don't have multiple simultaneous timeline event pulls at once for the case a pull took more than 2 seconds.
Use the RateLimiter class and perhaps a delay of only 200 ms to achieve that goal.
As you are now guaranteed to only have one pull at a time it would be safe to pull more frequently.

devtools/packages/devtools_app/lib/src/utils.dart

Line 452 in 554a0cb

class RateLimiter {

jacob314 · 2021-07-16T19:09:46Z

packages/devtools_app/lib/src/performance/performance_controller.dart

+        DateTime.now().millisecondsSinceEpoch,
+      );
+      allTraceEvents.add(eventWrapper);
+      debugTraceEventLog(eventWrapper.event.json.toString());


Can you remove this debugTraceEventLog or pass the raw json object instead of calling toString? This code will call the toString on the JSON even in release mode which could be somewhat expensive.

changing this method to be a callback so that the toString() won't get called unless the callback is called

jacob314

once case where fetching polls could take more than 2 secs is handled.

jacob314 · 2021-07-16T20:02:42Z

packages/devtools_app/lib/src/performance/performance_controller.dart

@@ -208,15 +210,17 @@ class PerformanceController extends DisposableController
      await processTraceEvents(allTraceEvents);
      _processing.value = false;

+      _timelinePollingRateLimiter = RateLimiter(
+        timelinePollingRateLimit,
+        () async => await _pullTraceEventsFromVmTimeline(),


replace:
() async => await _pullTraceEventsFromVmTimeline()
with
_pullTraceEventFromVmTimeline

@pq is there a lint tracking this case?

Hrmmm, closest thing would be unnecessary_lambdas.

packages/devtools_app/lib/src/performance/performance_controller.dart

kenzieschmoll added 11 commits May 28, 2021 07:49

revert this

05fce1a

Merge branch 'master' of github.com:flutter/devtools into frametiming

8e58bd4

Merge branch 'master' of github.com:flutter/devtools into frametiming

9d72913

checkpoint - revert this

0884d4e

Merge branch 'master' of github.com:flutter/devtools into frametiming

34827f0

Merge branch 'master' of github.com:flutter/devtools into frametiming

2731853

Merge branch 'master' of github.com:flutter/devtools into frametiming

76a0ac3

Merge branch 'master' of github.com:flutter/devtools into frametiming

175ec04

Merge branch 'master' of github.com:flutter/devtools into frametiming

f88de6a

Migrate performance page to FrameTiming API

2b0a8a6

cleanup

9d05ef6

kenzieschmoll requested a review from jacob314 June 28, 2021 21:51

jacob314 reviewed Jun 29, 2021

View reviewed changes

packages/devtools_app/lib/src/common_widgets.dart Outdated Show resolved Hide resolved

kenzieschmoll added 2 commits June 29, 2021 10:09

Merge branch 'master' of github.com:flutter/devtools into frametiming

4f0437e

merge master

9d03714

jacob314 reviewed Jun 29, 2021

View reviewed changes

packages/devtools_app/lib/src/performance/performance_controller.dart Outdated Show resolved Hide resolved

jacob314 reviewed Jun 29, 2021

View reviewed changes

Review comments

59511b5

kenzieschmoll commented Jun 29, 2021

View reviewed changes

Merge branch 'master' of github.com:flutter/devtools into frametiming

2881464

kenzieschmoll and others added 4 commits July 12, 2021 16:26

fix test

01edfa9

Merge branch 'master' into frametiming

8f05605

formatting

d52af23

Use frame copies in test instead of original test data

cc29f3b

kenzieschmoll and others added 11 commits July 13, 2021 09:27

fix broken test

667c6fe

Poll for new timeline events instead of listening to stream; edge cas…

6fbc90c

…e handling

Merge branch 'master' into frametiming

4ce60f5

fix test

0465619

Merge branch 'frametiming' of github.com:kenzieschmoll/devtools into …

3601195

…frametiming

fix a couple more tests and format

969af92

formatting

033fd4c

Fix performance screen test

84ca0ed

Merge branch 'master' of github.com:flutter/devtools into frametiming

e3f3cb3

Fix golden test

8d98f13

update golden image

2828b8c

jacob314 reviewed Jul 16, 2021

View reviewed changes

kenzieschmoll added 2 commits July 16, 2021 12:35

review comments

bb708e8

fix rate limiter

bb6c4a6

jacob314 reviewed Jul 16, 2021

View reviewed changes

packages/devtools_app/lib/src/performance/performance_controller.dart Outdated Show resolved Hide resolved

kenzieschmoll added 5 commits July 16, 2021 13:52

more tweaks

5bde5ca

Merge branch 'master' of github.com:flutter/devtools into frametiming

84be80c

fix lint

4dcfed2

fix broken test

700d3d1

test fix

5189d23

kenzieschmoll merged commit 6fe6871 into flutter:master Jul 16, 2021

kenzieschmoll deleted the frametiming branch July 16, 2021 22:21

Make Flutter Frame timeline live and migrate to the FrameTiming API #3168

Make Flutter Frame timeline live and migrate to the FrameTiming API #3168

Conversation

kenzieschmoll commented Jun 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob314 Jun 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenzieschmoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob314 commented Jul 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob314 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenzieschmoll commented Jun 28, 2021 •

edited

jacob314 Jun 29, 2021 •

edited