Add "Load all CPU samples" button to the CPU profiler #2943
Conversation
| return Row( | ||
| mainAxisAlignment: MainAxisAlignment.end, | ||
| children: [ | ||
| RefreshButton( |
There was a problem hiding this comment.
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?"
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
What's sufficient for a "timestamp" in this case? getVMTimelineMicros returns the approximate time the VM has been running in microseconds.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Is this another case where having frame number of the CPU traces themselves would solve the problem?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
FWIW this is landing here: flutter/flutter#82934
|
I'm going to land this PR as a temporary solution to this use case. Once @bkonyi has completed the necessary work in DDS I will work on the improved implementation for profiling app start. |
This will allow for profiling app start, since clicking this button pulls all the available CPU samples the VM has access to.
This CL also refactors some of the cpu profiler code to use widgets instead of helper methods