-
Notifications
You must be signed in to change notification settings - Fork 336
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
Cache CPU profiles for selected frames #3121
Cache CPU profiles for selected frames #3121
Conversation
@@ -61,6 +61,8 @@ class PerformanceController | |||
ValueListenable<List<FlutterFrame>> get flutterFrames => _flutterFrames; | |||
final _flutterFrames = ValueNotifier<List<FlutterFrame>>([]); | |||
|
|||
final _flutterFramesById = <String, FlutterFrame>{}; |
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 changes in this file are duplicated from performance/legacy/performance_controller.dart. These two files will diverge soon once the files under performance/ are migrated to use the FrameTiming API for flutter frames, though changes like this will continue to need to be applied to both files until the changes we need to use the FrameTiming API hit flutter stable.
packages/devtools_app/lib/src/performance/performance_controller.dart
Outdated
Show resolved
Hide resolved
_store[time] = profile; | ||
} | ||
|
||
void _maybeGenerateSubProfile(TimeRange time) { |
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: this is generating a bunch of overlapping profiles in the cache. That would make it a bit harder to later serialize this. Is computing the sub profiles expensive enough that it worth it to add completely overlapping ranges to the cache. It makes sense we will need some overlap as frames overlap but having completely overlapping entries seems a little odd.
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.
when a sub profile is generated, it has to be created (not expensive) and then processed - this is the expensive part. So if we don't cache each subprofile, there will be processing cost for each time we have to regenerate the sub profile from a super profile.
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 will prevent users from losing CPU profile data for frames they have already selected. We have seen users in user studies hit problems with CPU samples falling out of the CPU sample buffer.
We now cache profiles with their respective frames in the performance page. For timeline events within that frame, we will generate a sub profile from the frame's cached profile upon selection.
This PR also improves the performance of the CPU profiler by caching the
callTreeRoots
andbottomUpRoots
for an instance ofCpuProfileData
.This will improve #2631