-
Notifications
You must be signed in to change notification settings - Fork 29.7k
expose the debugProfileBuildsEnabled flag as a service extension #21492
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
expose the debugProfileBuildsEnabled flag as a service extension #21492
Conversation
|
Looks like 2 test failures: |
| name: 'debugProfileBuilds', | ||
| getter: () => new Future<bool>.value(debugProfileBuildsEnabled), | ||
| setter: (bool value) async { | ||
| if (debugProfileBuildsEnabled != value) |
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.
wrap adding this extension in a
assert(() {
...
return true;
}())
as it only does something useful in debug builds.
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.
or switch all uses of debugProfileBuilds to use profile(...) instead of assert(...)
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.
and change the name of the flag to indicate to clarify that it also works in profile builds.
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'd like this to be accessible from both debug and profile builds. For now, I may wrap this in an assert - so it's just available from debug builds - and plan to move at some point to using profile() and changing the callsites.
In the interim, perhaps I'll change the text name re register it as to something that won't need to change when it's also available in profile mode. So, from debugProfileBuilds to something like profileWidgetBuilds or similar.
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.
debugProfileBuildsEnabled should never be enabled in profile builds. It destroys performance. There's no point in running it in profile builds, you won't get useful data.
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.
Many performance problem diagnosis tools are still useful even though they hurt performance while enabled. For example, a CPU profiler that requires significant instrumentation to gather data.
We just need to make sure that users are clear when they have turned on service extensions in profile builds that make their performance non-representative of release performance. Perhaps we need to add a "SLOW" banner. :)
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.
It's not that it's non-representative of release performance, it's that the numbers aren't even proportional to release performance. The numbers are of zero value. All you get out of this flag is a bunch of stack traces, essentially. It's useful for knowing what work is happening, but not for any sort of performance-based tracking. That's why the flag is disabled in profile builds. You don't get anything better running it in profile builds than in debug builds.
Profile builds exist only so that you can get valuable release-proportional performance metrics. We shouldn't do anything in profile builds that muddles that message. If someone just wants to debug, they should use debug mode.
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.
It's useful for knowing what work is happening
I'd see it as an additional tool to use when trying to track down a performance issue. Say you're running in profile mode - for representative performance - and saw an issue. You could enable debugProfileBuildsEnabled, grab the output of a frame or two, and disable debugProfileBuildsEnabled. It would show you want that frame was doing, even if the specific perf from that frame was no longer reliable.
This PR doesn't switch the flag to being available in profile mode - it's still just in debug. I think we should just evaluate how useful it is in that mode for now.
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.
@Hixie knowing what work is happening is exactly what users often need to do to solve the simple performance problems we are tackling this quarter. I agree that they can diagnose those issues just fine in debug mode. The problem is the user doesn't realize what issue a certain portion of their app has until they use a profile build. Using a debug build to determine performance issues, users will become concerned about performance issues that are not actually issues in release builds risking premature optimizations.
There is another solution if users are confused by being able turn on performance debugging tools in profile build. We can work on making it faster for a user to switch back and forth between release and profile builds potentially keeping both running at the same time on the same device within a single application on Android.
|
ptal - some updates |
|
Looks like some checks are failing with your latest patch. |
|
Yup, thanks: will update - |
| // Expose the ability to send Widget rebuilds to the as [Timeline] events. | ||
| registerBoolServiceExtension( | ||
| name: 'profileWidgetBuilds', | ||
| getter: () => Future<bool>.value(debugProfileBuildsEnabled), |
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.
any reason this isn't just
getter: () async => debugProfileBuildsEnabled,
?
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.
No reason - I'll simplify this.
| ); | ||
|
|
||
| assert(() { | ||
| // Expose the ability to send Widget rebuilds to the as [Timeline] events. |
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.
typo in comment. Remove "to the"
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.
will fix
…dle-fix * commit 'adcf226e2ac585700fde5706666db351622a08a6': Roll engine f3a3d0c..57fd394 (1 commits) (flutter#22176) expose the debugProfileBuildsEnabled flag as a service extension (flutter#21492) [H] Cleanup (flutter#21542) Roll engine cc3009c..f3a3d0c (5 commits) (flutter#22162) Roll engine a8890fd to 8471862 (flutter#22153)
debugProfileBuildsEnabledflag as a service extensionThis will make the flag available for clients to dynamically enable and disable, and will be useful for users when investigating perf issues.