[framework] dont allocate forgotten children set in profile/release mode (#94911)
diff --git a/dev/bots/analyze.dart b/dev/bots/analyze.dart index 5e77fe7..68dae54 100644 --- a/dev/bots/analyze.dart +++ b/dev/bots/analyze.dart
@@ -98,6 +98,9 @@ print('$clock Integration test timeouts...'); await verifyIntegrationTestTimeouts(flutterRoot); + print('$clock null initialized debug fields...'); + await verifyNullInitializedDebugExpensiveFields(flutterRoot); + // Ensure that all package dependencies are in sync. print('$clock Package dependencies...'); await runCommand(flutter, <String>['update-packages', '--verify-only'], @@ -785,7 +788,7 @@ final Set<String> suggestions = <String>{}; final List<File> files = await _gitFiles(workingDirectory); for (final File file in files) { - if (path.basename(file.path).endsWith('_test.dart')) + if (path.basename(file.path).endsWith('_test.dart') || path.basename(file.path) == 'analyze.dart') continue; // Skip tests, they're not public-facing. final Uint8List bytes = file.readAsBytesSync(); // We allow invalid UTF-8 here so that binaries don't trip us up. @@ -1492,6 +1495,40 @@ } } +const String _kDebugOnlyAnnotation = '@_debugOnly'; +final RegExp _nullInitializedField = RegExp(r'kDebugMode \? [\w\<\> ,{}()]+ : null;'); + +Future<void> verifyNullInitializedDebugExpensiveFields(String workingDirectory, {int minimumMatches = 400}) async { + final String flutterLib = path.join(workingDirectory, 'packages', 'flutter', 'lib'); + final List<File> files = await _allFiles(flutterLib, 'dart', minimumMatches: minimumMatches) + .toList(); + final List<String> errors = <String>[]; + for (final File file in files) { + final List<String> lines = file.readAsLinesSync(); + for (int i = 0; i < lines.length; i += 1) { + final String line = lines[i]; + if (!line.contains(_kDebugOnlyAnnotation)) { + continue; + } + final String nextLine = lines[i + 1]; + if (_nullInitializedField.firstMatch(nextLine) == null) { + errors.add('${file.path} L$i'); + } + } + } + + if (errors.isNotEmpty) { + exitWithError(<String>[ + '${bold}ERROR: ${red}fields annotated with @_debugOnly must null initialize.$reset', + 'to ensure both the field and initializer are removed from profile/release mode.', + 'These fields should be written as:\n', + 'field = kDebugMode ? <DebugValue> : null;\n', + 'Errors were found in the following files:', + ...errors, + ]); + } +} + Future<CommandResult> _runFlutterAnalyze(String workingDirectory, { List<String> options = const <String>[], }) async {
diff --git a/dev/bots/test/analyze-test-input/root/packages/flutter/lib/bar.dart b/dev/bots/test/analyze-test-input/root/packages/flutter/lib/bar.dart new file mode 100644 index 0000000..52fe689 --- /dev/null +++ b/dev/bots/test/analyze-test-input/root/packages/flutter/lib/bar.dart
@@ -0,0 +1,18 @@ +// Copyright 2014 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +class _DebugOnly { + const _DebugOnly(); +} + +const _DebugOnly _debugOnly = _DebugOnly(); +const bool kDebugMode = bool.fromEnvironment('test-only'); + +class Foo { + @_debugOnly + final Map<String, String>? foo = kDebugMode ? <String, String>{} : null; + + @_debugOnly + final Map<String, String>? bar = kDebugMode ? null : <String, String>{}; +}
diff --git a/dev/bots/test/analyze_test.dart b/dev/bots/test/analyze_test.dart index 5ba5ff4..d554895 100644 --- a/dev/bots/test/analyze_test.dart +++ b/dev/bots/test/analyze_test.dart
@@ -193,4 +193,14 @@ }, )); }); + + test('analyze.dart - verifyNullInitializedDebugExpensiveFields', () async { + final String result = await capture(() => verifyNullInitializedDebugExpensiveFields( + testRootPath, + minimumMatches: 1, + ), exitCode: 1); + + expect(result, contains('L15')); + expect(result, isNot(contains('L12'))); + }); }
diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index 6bb84f2..1ad1e6d 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart
@@ -37,6 +37,22 @@ // abstract class FrogJar extends RenderObjectWidget { const FrogJar({Key? key}) : super(key: key); } // abstract class FrogJarParentData extends ParentData { late Size size; } +// An annotation used by test_analysis package to verify patterns are followed +// that allow for tree-shaking of both fields and their initializers. This +// annotation has no impact on code by itself, but indicates the following pattern +// should be followed for a given field: +// +// ```dart +// class Foo { +// final bar = kDebugMode ? Object() : null; +// } +// ``` +class _DebugOnly { + const _DebugOnly(); +} + +const _DebugOnly _debugOnly = _DebugOnly(); + // KEYS /// A key that is only equal to itself. @@ -2705,12 +2721,21 @@ } final Map<GlobalKey, Element> _globalKeyRegistry = <GlobalKey, Element>{}; - final Set<Element> _debugIllFatedElements = HashSet<Element>(); + + // In Profile/Release mode this field is initialized to `null`. The Dart compiler can + // eliminate unused fields, but not their initializers. + @_debugOnly + final Set<Element>? _debugIllFatedElements = kDebugMode ? HashSet<Element>() : null; + // This map keeps track which child reserves the global key with the parent. // Parent, child -> global key. // This provides us a way to remove old reservation while parent rebuilds the // child in the same slot. - final Map<Element, Map<Element, GlobalKey>> _debugGlobalKeyReservations = <Element, Map<Element, GlobalKey>>{}; + // + // In Profile/Release mode this field is initialized to `null`. The Dart compiler can + // eliminate unused fields, but not their initializers. + @_debugOnly + final Map<Element, Map<Element, GlobalKey>>? _debugGlobalKeyReservations = kDebugMode ? <Element, Map<Element, GlobalKey>>{} : null; /// The number of [GlobalKey] instances that are currently associated with /// [Element]s that have been built by this build owner. @@ -2720,7 +2745,7 @@ assert(() { assert(parent != null); assert(child != null); - _debugGlobalKeyReservations[parent]?.remove(child); + _debugGlobalKeyReservations?[parent]?.remove(child); return true; }()); } @@ -2732,7 +2757,7 @@ final Element oldElement = _globalKeyRegistry[key]!; assert(oldElement.widget != null); assert(element.widget.runtimeType != oldElement.widget.runtimeType); - _debugIllFatedElements.add(oldElement); + _debugIllFatedElements?.add(oldElement); } return true; }()); @@ -2757,8 +2782,8 @@ assert(() { assert(parent != null); assert(child != null); - _debugGlobalKeyReservations[parent] ??= <Element, GlobalKey>{}; - _debugGlobalKeyReservations[parent]![child] = key; + _debugGlobalKeyReservations?[parent] ??= <Element, GlobalKey>{}; + _debugGlobalKeyReservations?[parent]![child] = key; return true; }()); } @@ -2766,7 +2791,7 @@ void _debugVerifyGlobalKeyReservation() { assert(() { final Map<GlobalKey, Element> keyToParent = <GlobalKey, Element>{}; - _debugGlobalKeyReservations.forEach((Element parent, Map<Element, GlobalKey> childToKey) { + _debugGlobalKeyReservations?.forEach((Element parent, Map<Element, GlobalKey> childToKey) { // We ignore parent that are unmounted or detached. if (parent._lifecycleState == _ElementLifecycle.defunct || parent.renderObject?.attached == false) return; @@ -2829,7 +2854,7 @@ } }); }); - _debugGlobalKeyReservations.clear(); + _debugGlobalKeyReservations?.clear(); return true; }()); } @@ -2837,7 +2862,7 @@ void _debugVerifyIllFatedPopulation() { assert(() { Map<GlobalKey, Set<Element>>? duplicates; - for (final Element element in _debugIllFatedElements) { + for (final Element element in _debugIllFatedElements!) { if (element._lifecycleState != _ElementLifecycle.defunct) { assert(element != null); assert(element.widget != null); @@ -2851,7 +2876,7 @@ elements.add(_globalKeyRegistry[key]!); } } - _debugIllFatedElements.clear(); + _debugIllFatedElements?.clear(); if (duplicates != null) { final List<DiagnosticsNode> information = <DiagnosticsNode>[]; information.add(ErrorSummary('Multiple widgets used the same GlobalKey.')); @@ -2887,9 +2912,7 @@ Timeline.startSync('FINALIZE TREE', arguments: timelineArgumentsIndicatingLandmarkEvent); } try { - lockState(() { - _inactiveElements._unmountAll(); // this unregisters the GlobalKeys - }); + lockState(_inactiveElements._unmountAll); // this unregisters the GlobalKeys assert(() { try { _debugVerifyGlobalKeyReservation(); @@ -3588,8 +3611,8 @@ // never updates (the forgotten children are not removed from the tree // until the call to update happens) assert(() { - _debugForgottenChildrenWithGlobalKey.forEach(_debugRemoveGlobalKeyReservation); - _debugForgottenChildrenWithGlobalKey.clear(); + _debugForgottenChildrenWithGlobalKey?.forEach(_debugRemoveGlobalKeyReservation); + _debugForgottenChildrenWithGlobalKey?.clear(); return true; }()); _widget = newWidget; @@ -3795,7 +3818,11 @@ // The children that have been forgotten by forgetChild. This will be used in // [update] to remove the global key reservations of forgotten children. - final Set<Element> _debugForgottenChildrenWithGlobalKey = HashSet<Element>(); + // + // In Profile/Release mode this field is initialized to `null`. The Dart compiler can + // eliminate unused fields, but not their initializers. + @_debugOnly + final Set<Element>? _debugForgottenChildrenWithGlobalKey = kDebugMode ? HashSet<Element>() : null; /// Remove the given child from the element's child list, in preparation for /// the child being reused elsewhere in the element tree. @@ -3821,7 +3848,7 @@ // key duplication that we need to catch. assert(() { if (child.widget.key is GlobalKey) - _debugForgottenChildrenWithGlobalKey.add(child); + _debugForgottenChildrenWithGlobalKey?.add(child); return true; }()); }