Skip to content

Conversation

@devoncarew
Copy link
Contributor

  • expose the debugProfileBuildsEnabled flag as a service extension

This will make the flag available for clients to dynamically enable and disable, and will be useful for users when investigating perf issues.

@devoncarew
Copy link
Contributor Author

Looks like 2 test failures:

04:46 +3037 ~25 -1: /tmp/flutter sdk/packages/flutter/test/foundation/service_extensions_test.dart: Service extensions - debugProfileBuilds [E]                                                        
  Expected: {'enabled': 'true'}
    Actual: {'enabled': 'false'}
     Which: was 'false' instead of 'true' at location ['enabled']
  
  package:test                                   expect
  foundation/service_extensions_test.dart 307:5  main.<fn>
  ===== asynchronous gap ===========================
  dart:async                                     _AsyncAwaitCompleter.completeError
  foundation/service_extensions_test.dart        main.<fn>
  ===== asynchronous gap ===========================
  dart:async                                     _asyncThenWrapperHelper
  foundation/service_extensions_test.dart        main.<fn>
/tmp/flutter sdk/packages/flutter/test/foundation/service_extensions_test.dart: Service extensions - posttest [E]                                                                  
  Expected: <37>
    Actual: <38>
  
  package:test                                   expect
  foundation/service_extensions_test.dart 538:5  main.<fn>
  ===== asynchronous gap ===========================
  dart:async                                     _AsyncAwaitCompleter.completeError
  foundation/service_extensions_test.dart        main.<fn>

@devoncarew
Copy link
Contributor Author

@jacob314 @Hixie

name: 'debugProfileBuilds',
getter: () => new Future<bool>.value(debugProfileBuildsEnabled),
setter: (bool value) async {
if (debugProfileBuildsEnabled != value)
Copy link
Contributor

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.

Copy link
Contributor

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(...)

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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. :)

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@devoncarew
Copy link
Contributor Author

ptal - some updates

@jacob314
Copy link
Contributor

Looks like some checks are failing with your latest patch.

@devoncarew
Copy link
Contributor Author

Yup, thanks:

info • Unnecessary new keyword • packages/flutter/lib/src/widgets/binding.dart:304:23 • unnecessary_new

will update -

// Expose the ability to send Widget rebuilds to the as [Timeline] events.
registerBoolServiceExtension(
name: 'profileWidgetBuilds',
getter: () => Future<bool>.value(debugProfileBuildsEnabled),
Copy link
Contributor

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,

?

Copy link
Contributor Author

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.
Copy link
Contributor

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix

@devoncarew devoncarew merged commit 762c869 into flutter:master Sep 22, 2018
hereisderek added a commit to hereisderek/flutter that referenced this pull request Sep 27, 2018
…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)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants