From 35b18ba2e902dac1e16f557fa385069f1f5e1069 Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Tue, 5 Apr 2022 10:54:21 -0700 Subject: [PATCH] Made flag for debugging build time of user created widgets (#100926) * Added a bool that allows us to limit debugProfileBuildsEnabled to user created widgets. * made it turned on by default * switched to hashmap * Cleaned everything up and added tests * fixed an odd test where it wants to be able to add asserts and run in profile mode * hixie feedback * hixie2 * made it default to false * updated docstring as per dans request --- dev/tracing_tests/test/timeline_test.dart | 11 ++++ packages/flutter/lib/src/widgets/debug.dart | 19 ++++++- .../flutter/lib/src/widgets/framework.dart | 34 ++++++++---- .../lib/src/widgets/widget_inspector.dart | 52 ++++++++++++++----- .../test/widgets/widget_inspector_test.dart | 40 ++++++++++++++ .../lib/src/build_system/targets/common.dart | 2 +- .../build_system/targets/common_test.dart | 6 ++- 7 files changed, 139 insertions(+), 25 deletions(-) diff --git a/dev/tracing_tests/test/timeline_test.dart b/dev/tracing_tests/test/timeline_test.dart index 2d1c37491a..10306e33dd 100644 --- a/dev/tracing_tests/test/timeline_test.dart +++ b/dev/tracing_tests/test/timeline_test.dart @@ -110,6 +110,17 @@ void main() { expect(args['color'], 'Color(0xffffffff)'); debugProfileBuildsEnabled = false; + debugProfileBuildsEnabledUserWidgets = true; + await runFrame(() { TestRoot.state.updateWidget(Placeholder(key: UniqueKey(), color: const Color(0xFFFFFFFF))); }); + events = await fetchInterestingEvents(interestingLabels); + expect( + events.map(eventToName), + ['BUILD', 'Placeholder', 'LAYOUT', 'UPDATING COMPOSITING BITS', 'PAINT', 'COMPOSITING', 'FINALIZE TREE'], + ); + args = (events.where((TimelineEvent event) => event.json!['name'] == '$Placeholder').single.json!['args'] as Map).cast(); + expect(args['color'], 'Color(0xffffffff)'); + debugProfileBuildsEnabledUserWidgets = false; + debugProfileLayoutsEnabled = true; await runFrame(() { TestRoot.state.updateWidget(Placeholder(key: UniqueKey())); }); events = await fetchInterestingEvents(interestingLabels); diff --git a/packages/flutter/lib/src/widgets/debug.dart b/packages/flutter/lib/src/widgets/debug.dart index be19d6b60e..6688e1fb14 100644 --- a/packages/flutter/lib/src/widgets/debug.dart +++ b/packages/flutter/lib/src/widgets/debug.dart @@ -114,8 +114,24 @@ bool debugPrintGlobalKeyedWidgetLifecycle = false; /// * [debugProfileLayoutsEnabled], which does something similar for layout, /// and [debugPrintLayouts], its console equivalent. /// * [debugProfilePaintsEnabled], which does something similar for painting. +/// * [debugProfileBuildsEnabledUserWidgets], which adds events for user-created +/// [Widget] build times and incurs less overhead. bool debugProfileBuildsEnabled = false; +/// Adds [Timeline] events for every user-created [Widget] built. +/// +/// A user-created [Widget] is any [Widget] that is constructed in the root +/// library. Often [Widget]s contain child [Widget]s that are constructed in +/// libraries (for example, a [TextButton] having a [RichText] child). Timeline +/// events for those children will be omitted with this flag. This works for any +/// [Widget] not just ones declared in the root library. +/// +/// See also: +/// +/// * [debugProfileBuildsEnabled], which functions similarly but shows events +/// for every widget and has a higher overhead cost. +bool debugProfileBuildsEnabledUserWidgets = false; + /// Show banners for deprecated widgets. bool debugHighlightDeprecatedWidgets = false; @@ -423,7 +439,8 @@ bool debugAssertAllWidgetVarsUnset(String reason) { debugPrintScheduleBuildForStacks || debugPrintGlobalKeyedWidgetLifecycle || debugProfileBuildsEnabled || - debugHighlightDeprecatedWidgets) { + debugHighlightDeprecatedWidgets || + debugProfileBuildsEnabledUserWidgets) { throw FlutterError(reason); } return true; diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index 76b24b6f18..ac3673efe4 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -14,6 +14,7 @@ import 'debug.dart'; import 'focus_manager.dart'; import 'inherited_model.dart'; import 'notification_listener.dart'; +import 'widget_inspector.dart'; export 'package:flutter/foundation.dart' show factory, @@ -2640,10 +2641,13 @@ class BuildOwner { } return true; }()); - if (!kReleaseMode && debugProfileBuildsEnabled) { + final bool isTimelineTracked = !kReleaseMode && _isProfileBuildsEnabledFor(element.widget); + if (isTimelineTracked) { Map debugTimelineArguments = timelineArgumentsIndicatingLandmarkEvent; assert(() { - debugTimelineArguments = element.widget.toDiagnosticsNode().toTimelineArguments(); + if (kDebugMode) { + debugTimelineArguments = element.widget.toDiagnosticsNode().toTimelineArguments(); + } return true; }()); Timeline.startSync( @@ -2668,7 +2672,7 @@ class BuildOwner { ], ); } - if (!kReleaseMode && debugProfileBuildsEnabled) + if (isTimelineTracked) Timeline.finishSync(); index += 1; if (dirtyCount < _dirtyElements.length || _dirtyElementsNeedsResorting!) { @@ -3078,6 +3082,12 @@ class _NotificationNode { } } +bool _isProfileBuildsEnabledFor(Widget widget) { + return debugProfileBuildsEnabled || + (debugProfileBuildsEnabledUserWidgets && + debugIsWidgetLocalCreation(widget)); +} + /// An instantiation of a [Widget] at a particular location in the tree. /// /// Widgets describe how to configure a subtree but the same widget can be used @@ -3503,10 +3513,13 @@ abstract class Element extends DiagnosticableTree implements BuildContext { } else if (hasSameSuperclass && Widget.canUpdate(child.widget, newWidget)) { if (child.slot != newSlot) updateSlotForChild(child, newSlot); - if (!kReleaseMode && debugProfileBuildsEnabled) { + final bool isTimelineTracked = !kReleaseMode && _isProfileBuildsEnabledFor(newWidget); + if (isTimelineTracked) { Map debugTimelineArguments = timelineArgumentsIndicatingLandmarkEvent; assert(() { - debugTimelineArguments = newWidget.toDiagnosticsNode().toTimelineArguments(); + if (kDebugMode) { + debugTimelineArguments = newWidget.toDiagnosticsNode().toTimelineArguments(); + } return true; }()); Timeline.startSync( @@ -3515,7 +3528,7 @@ abstract class Element extends DiagnosticableTree implements BuildContext { ); } child.update(newWidget); - if (!kReleaseMode && debugProfileBuildsEnabled) + if (isTimelineTracked) Timeline.finishSync(); assert(child.widget == newWidget); assert(() { @@ -3765,10 +3778,13 @@ abstract class Element extends DiagnosticableTree implements BuildContext { Element inflateWidget(Widget newWidget, Object? newSlot) { assert(newWidget != null); - if (!kReleaseMode && debugProfileBuildsEnabled) { + final bool isTimelineTracked = !kReleaseMode && _isProfileBuildsEnabledFor(newWidget); + if (isTimelineTracked) { Map debugTimelineArguments = timelineArgumentsIndicatingLandmarkEvent; assert(() { - debugTimelineArguments = newWidget.toDiagnosticsNode().toTimelineArguments(); + if (kDebugMode) { + debugTimelineArguments = newWidget.toDiagnosticsNode().toTimelineArguments(); + } return true; }()); Timeline.startSync( @@ -3800,7 +3816,7 @@ abstract class Element extends DiagnosticableTree implements BuildContext { newChild.mount(this, newSlot); assert(newChild._lifecycleState == _ElementLifecycle.active); - if (!kReleaseMode && debugProfileBuildsEnabled) + if (isTimelineTracked) Timeline.finishSync(); return newChild; diff --git a/packages/flutter/lib/src/widgets/widget_inspector.dart b/packages/flutter/lib/src/widgets/widget_inspector.dart index c77e0f5417..d0f0a8b867 100644 --- a/packages/flutter/lib/src/widgets/widget_inspector.dart +++ b/packages/flutter/lib/src/widgets/widget_inspector.dart @@ -3,6 +3,7 @@ // found in the LICENSE file. import 'dart:async'; +import 'dart:collection' show HashMap; import 'dart:convert'; import 'dart:developer' as developer; import 'dart:math' as math; @@ -742,6 +743,8 @@ mixin WidgetInspectorService { int _nextId = 0; List? _pubRootDirectories; + /// Memoization for [_isLocalCreationLocation]. + final HashMap _isLocalCreationCache = HashMap(); bool _trackRebuildDirtyWidgets = false; bool _trackRepaintWidgets = false; @@ -1345,6 +1348,7 @@ mixin WidgetInspectorService { _pubRootDirectories = pubRootDirectories .map((String directory) => Uri.parse(directory).path) .toList(); + _isLocalCreationCache.clear(); } /// Set the [WidgetInspector] selection to the object matching the specified @@ -1518,14 +1522,11 @@ mixin WidgetInspectorService { if (creationLocation == null) { return false; } - return _isLocalCreationLocation(creationLocation); + return _isLocalCreationLocation(creationLocation.file); } - bool _isLocalCreationLocation(_Location? location) { - if (location == null) { - return false; - } - final String file = Uri.parse(location.file).path; + bool _isLocalCreationLocationImpl(String locationUri) { + final String file = Uri.parse(locationUri).path; // By default check whether the creation location was within package:flutter. if (_pubRootDirectories == null) { @@ -1541,6 +1542,17 @@ mixin WidgetInspectorService { return false; } + /// Memoized version of [_isLocalCreationLocationImpl]. + bool _isLocalCreationLocation(String locationUri) { + final bool? cachedValue = _isLocalCreationCache[locationUri]; + if (cachedValue != null) { + return cachedValue; + } + final bool result = _isLocalCreationLocationImpl(locationUri); + _isLocalCreationCache[locationUri] = result; + return result; + } + /// Wrapper around `json.encode` that uses a ring of cached values to prevent /// the Dart garbage collector from collecting objects between when /// the value is returned over the Observatory protocol and when the @@ -2066,7 +2078,7 @@ class _ElementLocationStatsTracker { entry = _LocationCount( location: location, id: id, - local: WidgetInspectorService.instance._isLocalCreationLocation(location), + local: WidgetInspectorService.instance._isLocalCreationLocation(location.file), ); if (entry.local) { newLocations.add(entry); @@ -3072,14 +3084,24 @@ bool debugIsLocalCreationLocation(Object object) { bool isLocal = false; assert(() { final _Location? location = _getCreationLocation(object); - if (location == null) - isLocal = false; - isLocal = WidgetInspectorService.instance._isLocalCreationLocation(location); + if (location != null) { + isLocal = WidgetInspectorService.instance._isLocalCreationLocation(location.file); + } return true; }()); return isLocal; } +/// Returns true if a [Widget] is user created. +/// +/// This is a faster variant of `debugIsLocalCreationLocation` that is available +/// in debug and profile builds but only works for [Widget]. +bool debugIsWidgetLocalCreation(Widget widget) { + final _Location? location = _getObjectCreationLocation(widget); + return location != null && + WidgetInspectorService.instance._isLocalCreationLocation(location.file); +} + /// Returns the creation location of an object in String format if one is available. /// /// ex: "file:///path/to/main.dart:4:3" @@ -3092,14 +3114,18 @@ String? _describeCreationLocation(Object object) { return location?.toString(); } +_Location? _getObjectCreationLocation(Object object) { + return object is _HasCreationLocation ? object._location : null; +} + /// Returns the creation location of an object if one is available. /// /// {@macro flutter.widgets.WidgetInspectorService.getChildrenSummaryTree} /// /// Currently creation locations are only available for [Widget] and [Element]. _Location? _getCreationLocation(Object? object) { - final Object? candidate = object is Element && !object.debugIsDefunct ? object.widget : object; - return candidate is _HasCreationLocation ? candidate._location : null; + final Object? candidate = object is Element && !object.debugIsDefunct ? object.widget : object; + return candidate == null ? null : _getObjectCreationLocation(candidate); } // _Location objects are always const so we don't need to worry about the GC @@ -3226,7 +3252,7 @@ class InspectorSerializationDelegate implements DiagnosticsSerializationDelegate if (creationLocation != null) { result['locationId'] = _toLocationId(creationLocation); result['creationLocation'] = creationLocation.toJsonMap(); - if (service._isLocalCreationLocation(creationLocation)) { + if (service._isLocalCreationLocation(creationLocation.file)) { _nodesCreatedByLocalProject.add(node); result['createdByLocalProject'] = true; } diff --git a/packages/flutter/test/widgets/widget_inspector_test.dart b/packages/flutter/test/widgets/widget_inspector_test.dart index 875342b0ea..c14a01791b 100644 --- a/packages/flutter/test/widgets/widget_inspector_test.dart +++ b/packages/flutter/test/widgets/widget_inspector_test.dart @@ -2998,6 +2998,46 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { expect(debugIsLocalCreationLocation(paddingElement.widget), isFalse); }, skip: !WidgetInspectorService.instance.isWidgetCreationTracked()); // [intended] Test requires --track-widget-creation flag. + testWidgets('debugIsWidgetLocalCreation test', (WidgetTester tester) async { + setupDefaultPubRootDirectory(service); + + final GlobalKey key = GlobalKey(); + + await tester.pumpWidget( + Directionality( + textDirection: TextDirection.ltr, + child: Container( + padding: const EdgeInsets.all(8), + child: Text('target', key: key, textDirection: TextDirection.ltr), + ), + ), + ); + + final Element element = key.currentContext! as Element; + expect(debugIsWidgetLocalCreation(element.widget), isTrue); + + final Finder paddingFinder = find.byType(Padding); + final Element paddingElement = paddingFinder.evaluate().first; + expect(debugIsWidgetLocalCreation(paddingElement.widget), isFalse); + }, skip: !WidgetInspectorService.instance.isWidgetCreationTracked()); // [intended] Test requires --track-widget-creation flag. + + testWidgets('debugIsWidgetLocalCreation false test', (WidgetTester tester) async { + final GlobalKey key = GlobalKey(); + + await tester.pumpWidget( + Directionality( + textDirection: TextDirection.ltr, + child: Container( + padding: const EdgeInsets.all(8), + child: Text('target', key: key, textDirection: TextDirection.ltr), + ), + ), + ); + + final Element element = key.currentContext! as Element; + expect(debugIsWidgetLocalCreation(element.widget), isFalse); + }, skip: WidgetInspectorService.instance.isWidgetCreationTracked()); // [intended] Test requires --no-track-widget-creation flag. + test('devToolsInspectorUri test', () { activeDevToolsServerAddress = 'http://127.0.0.1:9100'; connectedVmServiceUri = 'http://127.0.0.1:55269/798ay5al_FM=/'; diff --git a/packages/flutter_tools/lib/src/build_system/targets/common.dart b/packages/flutter_tools/lib/src/build_system/targets/common.dart index 6b0ac8d040..c0e78b7398 100644 --- a/packages/flutter_tools/lib/src/build_system/targets/common.dart +++ b/packages/flutter_tools/lib/src/build_system/targets/common.dart @@ -222,7 +222,7 @@ class KernelSnapshot extends Target { ), aot: buildMode.isPrecompiled, buildMode: buildMode, - trackWidgetCreation: trackWidgetCreation && buildMode == BuildMode.debug, + trackWidgetCreation: trackWidgetCreation && buildMode != BuildMode.release, targetModel: targetModel, outputFilePath: environment.buildDir.childFile('app.dill').path, packagesPath: packagesFile.path, diff --git a/packages/flutter_tools/test/general.shard/build_system/targets/common_test.dart b/packages/flutter_tools/test/general.shard/build_system/targets/common_test.dart index b9d2afde5e..31258f16f8 100644 --- a/packages/flutter_tools/test/general.shard/build_system/targets/common_test.dart +++ b/packages/flutter_tools/test/general.shard/build_system/targets/common_test.dart @@ -93,6 +93,7 @@ void main() { '--target=flutter', '--no-print-incremental-dependencies', ...buildModeOptions(BuildMode.profile, []), + '--track-widget-creation', '--aot', '--tfa', '--packages', @@ -109,7 +110,7 @@ void main() { expect(processManager, hasNoRemainingExpectations); }); - testWithoutContext('KernelSnapshot does not use track widget creation on profile builds', () async { + testWithoutContext('KernelSnapshot does use track widget creation on profile builds', () async { fileSystem.file('.dart_tool/package_config.json') ..createSync(recursive: true) ..writeAsStringSync('{"configVersion": 2, "packages":[]}'); @@ -129,6 +130,7 @@ void main() { '--target=flutter', '--no-print-incremental-dependencies', ...buildModeOptions(BuildMode.profile, []), + '--track-widget-creation', '--aot', '--tfa', '--packages', @@ -166,6 +168,7 @@ void main() { '--target=flutter', '--no-print-incremental-dependencies', ...buildModeOptions(BuildMode.profile, []), + '--track-widget-creation', '--aot', '--tfa', '--packages', @@ -204,6 +207,7 @@ void main() { '--target=flutter', '--no-print-incremental-dependencies', ...buildModeOptions(BuildMode.profile, []), + '--track-widget-creation', '--aot', '--tfa', '--packages',