From 8e2ca93f52d82d22df8b987cef74238ac53b4b28 Mon Sep 17 00:00:00 2001 From: Michael Goderbauer Date: Fri, 12 Oct 2018 21:07:55 -0700 Subject: [PATCH] Remove all service extensions from release mode (#23038) Service extensions can only be activated in debug or profile mode, their code should never be included in release mode. This PR adds guards around all service extension registration calls that enable Dart's tree shaker to remove the extension's code in release mode, which reduces our binary size: Android Snapshot (uncompressed): minus 127,384 Bytes (-124.40KB) APK (compressed): minus 38,136 Bytes (-37.24KB) iOS Snapshot (App.framework, uncompressed): 264,304 Bytes(-258.10KB) For details: https://docs.google.com/document/d/13JlgvliCn5sWwT2K2SfDwD1NhEfxpJH9DCf22gZZru8/edit **Benchmark Regressions:** This PR may cause benchmarks to regress because it may change the timing of GC. If you notice a benchmark regression **please note down the exact set of benchmarks that regressed on this PR** and then feel free to revert. I will follow-up with a PR that forces a GC before the effected benchmarks run to get a clean baseline before re-applying this PR. --- .../flutter/lib/src/foundation/binding.dart | 92 ++++++++++++++----- .../flutter/lib/src/rendering/binding.dart | 79 +++++++++------- .../flutter/lib/src/scheduler/binding.dart | 18 ++-- .../flutter/lib/src/services/binding.dart | 26 +++--- packages/flutter/lib/src/widgets/binding.dart | 80 ++++++++-------- .../lib/src/widgets/widget_inspector.dart | 59 ++++++------ .../foundation/service_extensions_test.dart | 32 +------ packages/flutter_tools/lib/src/vmservice.dart | 4 - 8 files changed, 206 insertions(+), 184 deletions(-) diff --git a/packages/flutter/lib/src/foundation/binding.dart b/packages/flutter/lib/src/foundation/binding.dart index 34e4f4269d8..e08df20263b 100644 --- a/packages/flutter/lib/src/foundation/binding.dart +++ b/packages/flutter/lib/src/foundation/binding.dart @@ -93,9 +93,7 @@ abstract class BindingBase { /// Implementations of this method must call their superclass /// implementation. /// - /// Service extensions are only exposed when the observatory is - /// included in the build, which should only happen in checked mode - /// and in profile mode. + /// {@macro flutter.foundation.bindingBase.registerServiceExtension} /// /// See also: /// @@ -104,18 +102,23 @@ abstract class BindingBase { @mustCallSuper void initServiceExtensions() { assert(!_debugServiceExtensionsRegistered); - registerSignalServiceExtension( - name: 'reassemble', - callback: reassembleApplication, - ); - registerSignalServiceExtension( - name: 'exit', - callback: _exitApplication, - ); - registerSignalServiceExtension( - name: 'frameworkPresent', - callback: () => Future.value(), - ); + + assert(() { + registerSignalServiceExtension( + name: 'reassemble', + callback: reassembleApplication, + ); + return true; + }()); + + const bool isReleaseMode = bool.fromEnvironment('dart.vm.product'); + if (!isReleaseMode) { + registerSignalServiceExtension( + name: 'exit', + callback: _exitApplication, + ); + } + assert(() { registerServiceExtension( name: 'platformOverride', @@ -239,6 +242,8 @@ abstract class BindingBase { /// no value. /// /// Calls the `callback` callback when the service extension is called. + /// + /// {@macro flutter.foundation.bindingBase.registerServiceExtension} @protected void registerSignalServiceExtension({ @required String name, @@ -267,6 +272,8 @@ abstract class BindingBase { /// /// Calls the `setter` callback with the new value when the /// service extension method is called with a new value. + /// + /// {@macro flutter.foundation.bindingBase.registerServiceExtension} @protected void registerBoolServiceExtension({ @required String name, @@ -297,6 +304,8 @@ abstract class BindingBase { /// /// Calls the `setter` callback with the new value when the /// service extension method is called with a new value. + /// + /// {@macro flutter.foundation.bindingBase.registerServiceExtension} @protected void registerNumericServiceExtension({ @required String name, @@ -326,6 +335,8 @@ abstract class BindingBase { /// /// Calls the `setter` callback with the new value when the /// service extension method is called with a new value. + /// + /// {@macro flutter.foundation.bindingBase.registerServiceExtension} @protected void registerStringServiceExtension({ @required String name, @@ -345,16 +356,51 @@ abstract class BindingBase { ); } - /// Registers a service extension method with the given name (full - /// name "ext.flutter.name"). The given callback is called when the - /// extension method is called. The callback must return a [Future] - /// that either eventually completes to a return value in the form - /// of a name/value map where the values can all be converted to - /// JSON using `json.encode()` (see [JsonEncoder]), or fails. In case of failure, the - /// failure is reported to the remote caller and is dumped to the - /// logs. + /// Registers a service extension method with the given name (full name + /// "ext.flutter.name"). + /// + /// The given callback is called when the extension method is called. The + /// callback must return a [Future] that either eventually completes to a + /// return value in the form of a name/value map where the values can all be + /// converted to JSON using `json.encode()` (see [JsonEncoder]), or fails. In + /// case of failure, the failure is reported to the remote caller and is + /// dumped to the logs. /// /// The returned map will be mutated. + /// + /// {@template flutter.foundation.bindingBase.registerServiceExtension} + /// A registered service extension can only be activated if the vm-service + /// is included in the build, which only happens in debug and profile mode. + /// Although a service extension cannot be used in release mode its code may + /// still be included in the Dart snapshot and blow up binary size if it is + /// not wrapped in a guard that allows the tree shaker to remove it (see + /// sample code below). + /// + /// ## Sample Code + /// + /// The following code registers a service extension that is only included in + /// debug builds: + /// + /// ```dart + /// assert(() { + /// // Register your service extension here. + /// return true; + /// }()); + /// + /// ``` + /// + /// A service extension registered with the following code snippet is + /// available in debug and profile mode: + /// + /// ```dart + /// if (!const bool.fromEnvironment('dart.vm.product')) { + // // Register your service extension here. + // } + /// ``` + /// + /// Both guards ensure that Dart's tree shaker can remove the code for the + /// service extension in release builds. + /// {@endTemplate} @protected void registerServiceExtension({ @required String name, diff --git a/packages/flutter/lib/src/rendering/binding.dart b/packages/flutter/lib/src/rendering/binding.dart index fd46141b28d..5f8bef1ffc7 100644 --- a/packages/flutter/lib/src/rendering/binding.dart +++ b/packages/flutter/lib/src/rendering/binding.dart @@ -55,7 +55,7 @@ abstract class RendererBinding extends BindingBase with ServicesBinding, Schedul super.initServiceExtensions(); assert(() { - // these service extensions only work in checked mode + // these service extensions only work in debug mode registerBoolServiceExtension( name: 'debugPaint', getter: () async => debugPaintSizeEnabled, @@ -64,51 +64,66 @@ abstract class RendererBinding extends BindingBase with ServicesBinding, Schedul return Future.value(); debugPaintSizeEnabled = value; return _forceRepaint(); - } + }, ); registerBoolServiceExtension( - name: 'debugPaintBaselinesEnabled', - getter: () async => debugPaintBaselinesEnabled, - setter: (bool value) { + name: 'debugPaintBaselinesEnabled', + getter: () async => debugPaintBaselinesEnabled, + setter: (bool value) { if (debugPaintBaselinesEnabled == value) return Future.value(); debugPaintBaselinesEnabled = value; return _forceRepaint(); - } + }, ); registerBoolServiceExtension( - name: 'repaintRainbow', - getter: () async => debugRepaintRainbowEnabled, - setter: (bool value) { - final bool repaint = debugRepaintRainbowEnabled && !value; - debugRepaintRainbowEnabled = value; - if (repaint) - return _forceRepaint(); - return Future.value(); - } + name: 'repaintRainbow', + getter: () async => debugRepaintRainbowEnabled, + setter: (bool value) { + final bool repaint = debugRepaintRainbowEnabled && !value; + debugRepaintRainbowEnabled = value; + if (repaint) + return _forceRepaint(); + return Future.value(); + }, + ); + registerSignalServiceExtension( + name: 'debugDumpLayerTree', + callback: () { + debugDumpLayerTree(); + return debugPrintDone; + }, ); return true; }()); - registerSignalServiceExtension( - name: 'debugDumpRenderTree', - callback: () { debugDumpRenderTree(); return debugPrintDone; } - ); + const bool isReleaseMode = bool.fromEnvironment('dart.vm.product'); + if (!isReleaseMode) { + // these service extensions work in debug or profile mode + registerSignalServiceExtension( + name: 'debugDumpRenderTree', + callback: () { + debugDumpRenderTree(); + return debugPrintDone; + }, + ); - registerSignalServiceExtension( - name: 'debugDumpLayerTree', - callback: () { debugDumpLayerTree(); return debugPrintDone; } - ); + registerSignalServiceExtension( + name: 'debugDumpSemanticsTreeInTraversalOrder', + callback: () { + debugDumpSemanticsTree(DebugSemanticsDumpOrder.traversalOrder); + return debugPrintDone; + }, + ); - registerSignalServiceExtension( - name: 'debugDumpSemanticsTreeInTraversalOrder', - callback: () { debugDumpSemanticsTree(DebugSemanticsDumpOrder.traversalOrder); return debugPrintDone; } - ); - - registerSignalServiceExtension( - name: 'debugDumpSemanticsTreeInInverseHitTestOrder', - callback: () { debugDumpSemanticsTree(DebugSemanticsDumpOrder.inverseHitTest); return debugPrintDone; } - ); + registerSignalServiceExtension( + name: 'debugDumpSemanticsTreeInInverseHitTestOrder', + callback: () { + debugDumpSemanticsTree(DebugSemanticsDumpOrder.inverseHitTest); + return debugPrintDone; + }, + ); + } } /// Creates a [RenderView] object to be the root of the diff --git a/packages/flutter/lib/src/scheduler/binding.dart b/packages/flutter/lib/src/scheduler/binding.dart index 61f8d17447e..98a6c2a9a86 100644 --- a/packages/flutter/lib/src/scheduler/binding.dart +++ b/packages/flutter/lib/src/scheduler/binding.dart @@ -207,13 +207,17 @@ abstract class SchedulerBinding extends BindingBase with ServicesBinding { @override void initServiceExtensions() { super.initServiceExtensions(); - registerNumericServiceExtension( - name: 'timeDilation', - getter: () async => timeDilation, - setter: (double value) async { - timeDilation = value; - } - ); + + const bool isReleaseMode = bool.fromEnvironment('dart.vm.product'); + if (!isReleaseMode) { + registerNumericServiceExtension( + name: 'timeDilation', + getter: () async => timeDilation, + setter: (double value) async { + timeDilation = value; + }, + ); + } } /// Whether the application is visible, and if so, whether it is currently diff --git a/packages/flutter/lib/src/services/binding.dart b/packages/flutter/lib/src/services/binding.dart index ebd2ae91106..fce5847be63 100644 --- a/packages/flutter/lib/src/services/binding.dart +++ b/packages/flutter/lib/src/services/binding.dart @@ -90,17 +90,21 @@ abstract class ServicesBinding extends BindingBase { @override void initServiceExtensions() { super.initServiceExtensions(); - registerStringServiceExtension( - // ext.flutter.evict value=foo.png will cause foo.png to be evicted from - // the rootBundle cache and cause the entire image cache to be cleared. - // This is used by hot reload mode to clear out the cache of resources - // that have changed. - name: 'evict', - getter: () async => '', - setter: (String value) async { - evict(value); - } - ); + + assert(() { + registerStringServiceExtension( + // ext.flutter.evict value=foo.png will cause foo.png to be evicted from + // the rootBundle cache and cause the entire image cache to be cleared. + // This is used by hot reload mode to clear out the cache of resources + // that have changed. + name: 'evict', + getter: () async => '', + setter: (String value) async { + evict(value); + }, + ); + return true; + }()); } /// Called in response to the `ext.flutter.evict` service extension. diff --git a/packages/flutter/lib/src/widgets/binding.dart b/packages/flutter/lib/src/widgets/binding.dart index 67bc1c4563e..463909f9721 100644 --- a/packages/flutter/lib/src/widgets/binding.dart +++ b/packages/flutter/lib/src/widgets/binding.dart @@ -267,37 +267,41 @@ abstract class WidgetsBinding extends BindingBase with SchedulerBinding, Gesture void initServiceExtensions() { super.initServiceExtensions(); - registerSignalServiceExtension( - name: 'debugDumpApp', - callback: () { - debugDumpApp(); - return debugPrintDone; - } - ); + const bool isReleaseMode = bool.fromEnvironment('dart.vm.product'); + if (!isReleaseMode) { + registerSignalServiceExtension( + name: 'debugDumpApp', + callback: () { + debugDumpApp(); + return debugPrintDone; + }, + ); - registerBoolServiceExtension( - name: 'showPerformanceOverlay', - getter: () => Future.value(WidgetsApp.showPerformanceOverlayOverride), - setter: (bool value) { - if (WidgetsApp.showPerformanceOverlayOverride == value) - return Future.value(); - WidgetsApp.showPerformanceOverlayOverride = value; - return _forceRebuild(); - } - ); - - registerBoolServiceExtension( - name: 'debugAllowBanner', - getter: () => Future.value(WidgetsApp.debugAllowBannerOverride), - setter: (bool value) { - if (WidgetsApp.debugAllowBannerOverride == value) - return Future.value(); - WidgetsApp.debugAllowBannerOverride = value; - return _forceRebuild(); - } - ); + registerBoolServiceExtension( + name: 'showPerformanceOverlay', + getter: () => + Future.value(WidgetsApp.showPerformanceOverlayOverride), + setter: (bool value) { + if (WidgetsApp.showPerformanceOverlayOverride == value) + return Future.value(); + WidgetsApp.showPerformanceOverlayOverride = value; + return _forceRebuild(); + }, + ); + } assert(() { + registerBoolServiceExtension( + name: 'debugAllowBanner', + getter: () => Future.value(WidgetsApp.debugAllowBannerOverride), + setter: (bool value) { + if (WidgetsApp.debugAllowBannerOverride == value) + return Future.value(); + WidgetsApp.debugAllowBannerOverride = value; + return _forceRebuild(); + }, + ); + // Expose the ability to send Widget rebuilds as [Timeline] events. registerBoolServiceExtension( name: 'profileWidgetBuilds', @@ -305,25 +309,13 @@ abstract class WidgetsBinding extends BindingBase with SchedulerBinding, Gesture setter: (bool value) async { if (debugProfileBuildsEnabled != value) debugProfileBuildsEnabled = value; - } + }, ); + + WidgetInspectorService.instance.initServiceExtensions(registerServiceExtension); + return true; }()); - - // This service extension is deprecated and will be removed by 7/1/2018. - // Use ext.flutter.inspector.show instead. - registerBoolServiceExtension( - name: 'debugWidgetInspector', - getter: () async => WidgetsApp.debugShowWidgetInspectorOverride, - setter: (bool value) { - if (WidgetsApp.debugShowWidgetInspectorOverride == value) - return Future.value(); - WidgetsApp.debugShowWidgetInspectorOverride = value; - return _forceRebuild(); - } - ); - - WidgetInspectorService.instance.initServiceExtensions(registerServiceExtension); } Future _forceRebuild() { diff --git a/packages/flutter/lib/src/widgets/widget_inspector.dart b/packages/flutter/lib/src/widgets/widget_inspector.dart index 0df0634aabd..3ea2f6fca47 100644 --- a/packages/flutter/lib/src/widgets/widget_inspector.dart +++ b/packages/flutter/lib/src/widgets/widget_inspector.dart @@ -921,13 +921,11 @@ class WidgetInspectorService { /// Called to register service extensions. /// - /// Service extensions are only exposed when the observatory is - /// included in the build, which should only happen in checked mode - /// and in profile mode. - /// /// See also: /// /// * + /// * [BindingBase.initServiceExtensions], which explains when service + /// extensions can be used. void initServiceExtensions( _RegisterServiceExtensionCallback registerServiceExtensionCallback) { _registerServiceExtensionCallback = registerServiceExtensionCallback; @@ -1027,36 +1025,33 @@ class WidgetInspectorService { name: 'isWidgetCreationTracked', callback: isWidgetCreationTracked, ); - assert(() { - registerServiceExtension( - name: 'screenshot', - callback: (Map parameters) async { - assert(parameters.containsKey('id')); - assert(parameters.containsKey('width')); - assert(parameters.containsKey('height')); + registerServiceExtension( + name: 'screenshot', + callback: (Map parameters) async { + assert(parameters.containsKey('id')); + assert(parameters.containsKey('width')); + assert(parameters.containsKey('height')); - final ui.Image image = await screenshot( - toObject(parameters['id']), - width: double.parse(parameters['width']), - height: double.parse(parameters['height']), - margin: parameters.containsKey('margin') ? - double.parse(parameters['margin']) : 0.0, - maxPixelRatio: parameters.containsKey('maxPixelRatio') ? - double.parse(parameters['maxPixelRatio']) : 1.0, - debugPaint: parameters['debugPaint'] == 'true', - ); - if (image == null) { - return {'result': null}; - } - final ByteData byteData = await image.toByteData(format:ui.ImageByteFormat.png); + final ui.Image image = await screenshot( + toObject(parameters['id']), + width: double.parse(parameters['width']), + height: double.parse(parameters['height']), + margin: parameters.containsKey('margin') ? + double.parse(parameters['margin']) : 0.0, + maxPixelRatio: parameters.containsKey('maxPixelRatio') ? + double.parse(parameters['maxPixelRatio']) : 1.0, + debugPaint: parameters['debugPaint'] == 'true', + ); + if (image == null) { + return {'result': null}; + } + final ByteData byteData = await image.toByteData(format:ui.ImageByteFormat.png); - return { - 'result': base64.encoder.convert(Uint8List.view(byteData.buffer)), - }; - }, - ); - return true; - }()); + return { + 'result': base64.encoder.convert(Uint8List.view(byteData.buffer)), + }; + }, + ); } /// Clear all InspectorService object references. diff --git a/packages/flutter/test/foundation/service_extensions_test.dart b/packages/flutter/test/foundation/service_extensions_test.dart index de2950fee44..e2ca5b9558a 100644 --- a/packages/flutter/test/foundation/service_extensions_test.dart +++ b/packages/flutter/test/foundation/service_extensions_test.dart @@ -359,13 +359,6 @@ void main() { expect(binding.extensions.containsKey('exit'), isTrue); }); - test('Service extensions - frameworkPresent', () async { - Map result; - - result = await binding.testExtension('frameworkPresent', {}); - expect(result, {}); - }); - test('Service extensions - platformOverride', () async { Map result; @@ -489,29 +482,6 @@ void main() { expect(binding.frameScheduled, isFalse); }); - test('Service extensions - debugWidgetInspector', () async { - Map result; - - expect(binding.frameScheduled, isFalse); - expect(WidgetsApp.debugShowWidgetInspectorOverride, false); - result = await binding.testExtension('debugWidgetInspector', {}); - expect(result, { 'enabled': 'false' }); - expect(WidgetsApp.debugShowWidgetInspectorOverride, false); - result = await binding.testExtension('debugWidgetInspector', { 'enabled': 'true' }); - expect(result, { 'enabled': 'true' }); - expect(WidgetsApp.debugShowWidgetInspectorOverride, true); - result = await binding.testExtension('debugWidgetInspector', {}); - expect(result, { 'enabled': 'true' }); - expect(WidgetsApp.debugShowWidgetInspectorOverride, true); - result = await binding.testExtension('debugWidgetInspector', { 'enabled': 'false' }); - expect(result, { 'enabled': 'false' }); - expect(WidgetsApp.debugShowWidgetInspectorOverride, false); - result = await binding.testExtension('debugWidgetInspector', {}); - expect(result, { 'enabled': 'false' }); - expect(WidgetsApp.debugShowWidgetInspectorOverride, false); - expect(binding.frameScheduled, isFalse); - }); - test('Service extensions - timeDilation', () async { Map result; @@ -541,7 +511,7 @@ void main() { // If you add a service extension... TEST IT! :-) // ...then increment this number. - expect(binding.extensions.length, 39); + expect(binding.extensions.length, 37); expect(console, isEmpty); debugPrint = debugPrintThrottled; diff --git a/packages/flutter_tools/lib/src/vmservice.dart b/packages/flutter_tools/lib/src/vmservice.dart index f83b50e16c4..d63fd82180c 100644 --- a/packages/flutter_tools/lib/src/vmservice.dart +++ b/packages/flutter_tools/lib/src/vmservice.dart @@ -1295,10 +1295,6 @@ class Isolate extends ServiceObjectOwner { ); } - Future flutterFrameworkPresent() async { - return await invokeFlutterExtensionRpcRaw('ext.flutter.frameworkPresent') != null; - } - Future> uiWindowScheduleFrame() async { return await invokeFlutterExtensionRpcRaw('ext.ui.window.scheduleFrame'); }