-
Notifications
You must be signed in to change notification settings - Fork 341
Add Queued Microtasks tab to Performance View #9239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
b3c3d89
to
d85106e
Compare
@@ -23,7 +23,11 @@ TODO: Remove this section if there are not any general updates. | |||
|
|||
## Performance updates | |||
|
|||
TODO: Remove this section if there are not any general updates. | |||
- Added a Queued Microtasks tab to the Performance View, which allows a user to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how these names should be capaitalized.
d85106e
to
0f3ea7e
Compare
final _status = ValueNotifier<QueuedMicrotasksControllerStatus>( | ||
QueuedMicrotasksControllerStatus.empty, | ||
); | ||
ValueListenable<QueuedMicrotasksControllerStatus> get status => _status; | ||
|
||
final _queuedMicrotasks = ValueNotifier<QueuedMicrotasks?>(null); | ||
ValueListenable<QueuedMicrotasks?> get queuedMicrotasks => _queuedMicrotasks; | ||
|
||
final _selectedMicrotask = ValueNotifier<Microtask?>(null); | ||
ValueListenable<Microtask?> get selectedMicrotask => _selectedMicrotask; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
order getters before private field: https://github.com/flutter/devtools/blob/master/STYLE.md#order-of-getters-and-setters
Future<void> handleSelectedFrame(FlutterFrame frame) async {} | ||
|
||
@override | ||
Future<void> setOfflineData(OfflinePerformanceData offlineData) async {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OfflinePerformanceData should be able to take the data from this tab and serialize it for offline exports of the performance page data. Then this method should set the _queuedMicrotasks
and _selectedMicrotask
from offline data when this method is called. See the other performance feature controllers for an example.
} | ||
} | ||
|
||
class QueuedMicrotasksTabControls extends StatelessWidget { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this helper widget seems unnecessary. Can we just use RefreshQueuedMicrotasksButton directly below where this widget is currently used?
// run earliest is at index 0 of the returned list. We use those indices of the | ||
// returned list to sort the entries of the microtask selector, so that they | ||
// they also appear in ascending order of when they will be dequeued. | ||
typedef IndexedMicrotask = (int, Microtask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we used named elements in this record so that we don't have to use $1
and $2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation should also be using ///
final void Function(Microtask?) _setSelectedMicrotask; | ||
|
||
@override | ||
Widget build(BuildContext context) => Column( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this column necessary or can it be removed?
const PerformanceScreenBody({ | ||
super.key, | ||
// This allows the body to modify the value that gets returned by the | ||
// enclosing [PerformanceScreen]'s `showIsolateSelector` [ValueListenable]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me wonder generally whether this feature belongs in the Performance screen or the CPU profiler screen. Can you explain the user journey of how a user would use this feature to debug a performance problem, and what type of performance problem would the user be encountering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only known use case of this feature is checking if there's a bug causing an isolate's microtask queue to get stuck. This was requested in b/329176605. The user journey of using this feature would look something like:
- The user notices that their app can become unresponsive
- The user determines a reproduction that can make their app enter an unresponsive state
- The user starts their app with
flutter run --profile-microtasks
and connects their app to DevTools - The user makes their app enter an unresponsive state
- The user navigates to the Queued Microtasks tab
- The user selects an isolate in the isolate selector
- The user takes a snapshot of the selected isolate's microtask queue
- The user notes down the ID of the microtask scheduled to run next
- The user waits for some amount of time (microtasks should drain quickly when the queue is not stuck, so waiting for a second is enough)
- The user takes another snapshot of the selected isolate's microtask queue
- If the ID of the microtask scheduled to run next is the same as the one noted down in step 8, there is a bug causing the microtask queue to be stuck
- If a bug was discovered in step 11, report it in the Dart SDK repo. Otherwise, repeat steps 6-12 until all isolates have been checked.
createMockPerformanceControllerWithDefaults(), | ||
); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after adding offline support for this feature, please add tests for setOfflineData here.
}); | ||
|
||
test('refresh', () async { | ||
await controller.refresh(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add expect statements for what the state should have been prior to calling refresh
await tester.tap(find.text('Timeline Events')); | ||
await tester.pumpAndSettle(); | ||
await tester.tap(find.text('Queued Microtasks')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have to switch tabs? we should just be able to call pumpAndSettle to trigger a rebuild, and if that doesn't work then maybe there is a bug?
@@ -1 +1 @@ | |||
36ea2bdeab611e908967b6fa57659998f600a2cb | |||
ead0a01bb9707b60097679a43a6262a8979f4a89 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use a version of Flutter that has been checked into google3. The latest is 7cfbf1b000546a88f7c1a5b0c154a51fd97147f8
.getQueuedMicrotasks(isolateId); | ||
_queuedMicrotasks.value = queuedMicrotasks; | ||
|
||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary return.
class RefreshQueuedMicrotasksButton extends StatelessWidget { | ||
const RefreshQueuedMicrotasksButton({ | ||
super.key, | ||
required QueuedMicrotasksController controller, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd just make _controller
public and use required this.controller
here.
// run earliest is at index 0 of the returned list. We use those indices of the | ||
// returned list to sort the entries of the microtask selector, so that they | ||
// they also appear in ascending order of when they will be dequeued. | ||
typedef IndexedMicrotask = (int, Microtask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation should also be using ///
); | ||
} | ||
|
||
class StackTraceView extends StatelessWidget { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/StackTraceView/MicrotaskStackTraceView/
@@ -16,6 +16,9 @@ const profiler = 'profiler'; | |||
// Defined in SDK: https://github.com/dart-lang/sdk/blob/master/runtime/vm/profiler.cc#L36 | |||
const profilePeriod = 'profile_period'; | |||
|
|||
// Defined in SDK: https://github.com/dart-lang/sdk/blob/main/runtime/vm/microtask_mirror_queues.cc#L18-L23. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's worth including the line numbers here, especially if we're using a URL that doesn't point to a specific commit.
vmFlags: [ | ||
( | ||
flagName: profileMicrotasks, | ||
value: shouldEnableMicrotaskProfiling ? 'true' : 'false', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: shouldEnableMicrotaskProfiling.toString()
This PR adds a Queued Microtasks tab to the Performance View, which allows a user to see details about the microtasks scheduled in an isolate's microtask queue. This tab currently only appears when DevTools is connected to a Flutter or Dart app started with
--profile-microtasks
.