Skip to content
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.

Sign up for GitHub

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

Add "Load all CPU samples" button to the CPU profiler #2943

Merged
merged 8 commits into from
May 20, 2021
Merged

Add "Load all CPU samples" button to the CPU profiler #2943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c5aad58
Add "Load all CPU samples" button to the CPU profiler
kenzieschmoll Apr 26, 2021
8292c4c
fix test
kenzieschmoll Apr 27, 2021
7f3cca5
Merge branch 'master' of github.com:flutter/devtools into profiler
kenzieschmoll Apr 27, 2021
eaf0f66
Merge branch 'master' into profiler
kenzieschmoll May 20, 2021
610423a
Remove empty line
kenzieschmoll May 20, 2021
6b1bc72
Remove other line
kenzieschmoll May 20, 2021
c5ef208
Remove white space
kenzieschmoll May 20, 2021
d5f86d8
Fix typo
kenzieschmoll May 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/devtools_app/lib/src/common_widgets.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,12 +174,13 @@ class ClearButton extends IconLabelButton {
class RefreshButton extends IconLabelButton {
const RefreshButton({
Key key,
String label = 'Refresh',
double includeTextWidth,
@required VoidCallback onPressed,
}) : super(
key: key,
icon: Icons.refresh,
label: 'Refresh',
label: label,
includeTextWidth: includeTextWidth,
onPressed: onPressed,
);
Expand Down
5 changes: 5 additions & 0 deletions packages/devtools_app/lib/src/profiler/profiler_screen.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,11 @@ class _SecondaryControls extends StatelessWidget {
return Row(
mainAxisAlignment: MainAxisAlignment.end,
children: [
RefreshButton(
Copy link
Contributor

Choose a reason for hiding this comment

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

Fyi @InMatrix. I'm not sure users will understand what "Load all CPU samples" does. Any ideas for a detailed tooltip and/or an alternate name that makes it clearer?
This is basically the refresh button from the Performance page but in addition to the existing record and pause buttons. What we are trying to say is.
"The VM already has N seconds of recorded CPU profile samples in a buffer of recent activity. Display that data on this page?"

Copy link

@InMatrix InMatrix Apr 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Do we expect this button to be used for debugging slow app start only? If that's the case, maybe giving it a name that reflects the user's goal, something like "Load samples from app start," and limit how much data is loaded (e.g., anything before first user interaction).

"The VM already has N seconds of recorded CPU profile samples in a buffer of recent activity. Display that data on this page?"

I wonder if the data would be misleading if the earliest available sample doesn't go all the way back to the app's start. For example, if the user clicks this button 10 minutes after they started their app, is there any guarantee that they'll still have samples from the start? Can we show timestamps relative to the app start time?

Copy link
Member Author

@kenzieschmoll kenzieschmoll Apr 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if the user clicks this button 10 minutes after they started their app, is there any guarantee that they'll still have samples from the start?

No, there is not. And if the user was actually interacting with their app, it's almost certain that we will not have samples from app start.

Can we show timestamps relative to the app start time?

This would be possible if we had a way to access the timestamp of app start. We could probably get close by taking the timestamp of the first timeline event we have, but that wouldn't be entirely accurate. @bkonyi do you know how we could get the timestamp of app start?

What if the button said "Load all available data" and then the tooltip said something more detailed like, "Load all CPU profile samples available in the VM sample buffer. This will include CPU samples from app start if this button is clicked immediately after your app finishes startup."

Copy link
Contributor

Choose a reason for hiding this comment

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

What's sufficient for a "timestamp" in this case? getVMTimelineMicros returns the approximate time the VM has been running in microseconds.

Copy link

Choose a reason for hiding this comment

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

Is there a use case for loading all available data after the data from app start is gone? If this button won't be useful in that situation, it's probably better to disable that and show a tooltip explaining the intended workflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

As a temporary solution while we finalize a more robust design using DDS, I'd like to change the button text to "Profile app startup", and manually request a profile in DevTools from time 0 to the timestamp of the Flutter.FirstFrame event. This runs the risk that samples in that timeframe will not be available, and if that happens, we can show a description for why that is. When we move to a more robust solution, this problem will go away, but this could give users something to work with for now.

For Dart CLI apps, we can request the first 30 seconds or just not show the button.

Copy link
Member Author

Choose a reason for hiding this comment

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

Running into another problem here where the Flutter.Frame and Flutter.FirstFrame event timestamps don't match the timestamps that the VM uses for timeline events and CPU samples. @bkonyi @jonahwilliams ideas for ways around this?

Copy link
Member

Choose a reason for hiding this comment

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

Is this another case where having frame number of the CPU traces themselves would solve the problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we had a 0-based or 1-based frame numbering system, then we could look for the 0th or 1st frame. If we couldn't guarantee what the first frame number would be, then this wouldn't work, but if we can, then that would solve it.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW this is landing here: flutter/flutter#82934

label: 'Load all CPU samples',
onPressed: controller.loadAllSamples,
),
const SizedBox(width: defaultSpacing),
ProfileGranularityDropdown(
screenId: ProfilerScreen.id,
profileGranularityFlagNotifier:
Expand Down
10 changes: 10 additions & 0 deletions packages/devtools_app/lib/src/profiler/profiler_screen_controller.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,16 @@ class ProfilerScreenController with CpuProfilerControllerProviderMixin {
);
}

Future<void> loadAllSamples() async {
cpuProfilerController.reset();
await cpuProfilerController.pullAndProcessProfile(
startMicros: 0,
// Using [maxJsInt] as [extentMicros] for the getCpuProfile requests will
// give us all cpu samples we have available
extentMicros: maxJsInt,
);
}

/// Exports the current profiler data to a .json file.
///
/// This method returns the name of the file that was downloaded.
Expand Down