[H] Move the splitting of licenses to an isolate (#14160) * Move the splitting of licenses to an isolate This improves (from horrific to terrible) the performance of the license screen. It also introduces a feature in the foundation layer to make using isolates for one-off computations easier. The real problem that remains with this, though, is that transfering data between isolates is a stop-the-world operation and can take an absurd amount of time (far more than a few milliseconds), so we still skip frames. More work thus remains to be done. * - Add profile instrumentation to the isolate compute() method - Add profile instrumentation to the LicensePage - Add profile instrumentation to the scheduleTask method - Make scheduleTask support returning a value - Make the license page builder logic use scheduled tasks so that it doesn't blow the frame budget
diff --git a/dev/bots/analyze-sample-code.dart b/dev/bots/analyze-sample-code.dart index 4ff9210..a82c44c 100644 --- a/dev/bots/analyze-sample-code.dart +++ b/dev/bots/analyze-sample-code.dart
@@ -37,11 +37,16 @@ final List<String> code; final String postamble; Iterable<String> get strings sync* { - if (preamble != null) + if (preamble != null) { + assert(!preamble.contains('\n')); yield preamble; + } + assert(!code.any((String line) => line.contains('\n'))); yield* code; - if (postamble != null) + if (postamble != null) { + assert(!postamble.contains('\n')); yield postamble; + } } List<Line> get lines { final List<Line> result = new List<Line>.generate(code.length, (int index) => start + index); @@ -61,6 +66,7 @@ final Directory temp = Directory.systemTemp.createTempSync('analyze_sample_code_'); int exitCode = 1; bool keepMain = false; + final List<String> buffer = <String>[]; try { final File mainDart = new File(path.join(temp.path, 'main.dart')); final File pubSpec = new File(path.join(temp.path, 'pubspec.yaml')); @@ -128,7 +134,6 @@ } } } - final List<String> buffer = <String>[]; buffer.add('// generated code'); buffer.add('import \'dart:async\';'); buffer.add('import \'dart:math\' as math;'); @@ -146,6 +151,7 @@ buffer.addAll(section.strings); lines.addAll(section.lines); } + assert(buffer.length == lines.length); mainDart.writeAsStringSync(buffer.join('\n')); pubSpec.writeAsStringSync(''' name: analyze_sample_code @@ -180,17 +186,23 @@ final String message = error.substring(start + kBullet.length, end); final String atMatch = atRegExp.firstMatch(error)[0]; final int colon2 = error.indexOf(kColon, end + atMatch.length); - if (colon2 < 0) + if (colon2 < 0) { + keepMain = true; throw 'failed to parse error message: $error'; + } final String line = error.substring(end + atMatch.length, colon2); final int bullet2 = error.indexOf(kBullet, colon2); - if (bullet2 < 0) + if (bullet2 < 0) { + keepMain = true; throw 'failed to parse error message: $error'; + } final String column = error.substring(colon2 + kColon.length, bullet2); final int lineNumber = int.parse(line, radix: 10, onError: (String source) => throw 'failed to parse error message: $error'); final int columnNumber = int.parse(column, radix: 10, onError: (String source) => throw 'failed to parse error message: $error'); - if (lineNumber < 0 || lineNumber >= lines.length) - throw 'failed to parse error message: $error'; + if (lineNumber < 1 || lineNumber > lines.length) { + keepMain = true; + throw 'failed to parse error message (read line number as $lineNumber; total number of lines is ${lines.length}): $error'; + } final Line actualLine = lines[lineNumber - 1]; final String errorCode = error.substring(bullet2 + kBullet.length); if (errorCode == 'unused_element') { @@ -211,6 +223,7 @@ } } else { print('?? $error'); + keepMain = true; errorCount += 1; } } @@ -222,6 +235,13 @@ } finally { if (keepMain) { print('Kept ${temp.path} because it had errors (see above).'); + print('-------8<-------'); + int number = 1; + for (String line in buffer) { + print('${number.toString().padLeft(6, " ")}: $line'); + number += 1; + } + print('-------8<-------'); } else { temp.deleteSync(recursive: true); }
diff --git a/packages/flutter/lib/foundation.dart b/packages/flutter/lib/foundation.dart index 0fc5096..fa376b0 100644 --- a/packages/flutter/lib/foundation.dart +++ b/packages/flutter/lib/foundation.dart
@@ -39,6 +39,7 @@ export 'src/foundation/collections.dart'; export 'src/foundation/debug.dart'; export 'src/foundation/diagnostics.dart'; +export 'src/foundation/isolates.dart'; export 'src/foundation/key.dart'; export 'src/foundation/licenses.dart'; export 'src/foundation/node.dart';
diff --git a/packages/flutter/lib/src/foundation/isolates.dart b/packages/flutter/lib/src/foundation/isolates.dart new file mode 100644 index 0000000..3cf9d7d --- /dev/null +++ b/packages/flutter/lib/src/foundation/isolates.dart
@@ -0,0 +1,103 @@ +// Copyright 2018 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:async'; +import 'dart:developer' show Timeline, Flow; +import 'dart:isolate'; + +import 'package:meta/meta.dart'; + +import 'profile.dart'; + +/// Signature for the callback passed to [compute]. +/// +/// {@macro flutter.foundation.compute.types} +/// +/// Instances of [ComputeCallback] must be top-level functions or static methods +/// of classes, not closures or instance methods of objects. +/// +/// {@macro flutter.foundation.compute.limitations} +typedef R ComputeCallback<Q, R>(Q message); + +/// Spawn an isolate, run `callback` on that isolate, passing it `message`, and +/// (eventually) return the value returned by `callback`. +/// +/// This is useful for operations that take longer than a few milliseconds, and +/// which would therefore risk skipping frames. For tasks that will only take a +/// few milliseconds, consider [scheduleTask] instead. +/// +/// {@template flutter.foundation.compute.types} +/// `Q` is the type of the message that kicks off the computation. +/// +/// `R` is the type of the value returned. +/// {@endtemplate} +/// +/// The `callback` argument must be a top-level function, not a closure or an +/// instance or static method of a class. +/// +/// {@template flutter.foundation.compute.limitations} +/// There are limitations on the values that can be sent and received to and +/// from isolates. These limitations constrain the values of `Q` and `R` that +/// are possible. See the discussion at [SendPort.send]. +/// {@endtemplate} +/// +/// The `debugLabel` argument can be specified to provide a name to add to the +/// [Timeline]. This is useful when profiling an application. +Future<R> compute<Q, R>(ComputeCallback<Q, R> callback, Q message, { String debugLabel }) async { + profile(() { debugLabel ??= callback.toString(); }); + final Flow flow = Flow.begin(); + Timeline.startSync('$debugLabel: start', flow: flow); + final ReceivePort resultPort = new ReceivePort(); + Timeline.finishSync(); + final Isolate isolate = await Isolate.spawn( + _spawn, + new _IsolateConfiguration<Q, R>( + callback, + message, + resultPort.sendPort, + debugLabel, + flow.id, + ), + errorsAreFatal: true, + onExit: resultPort.sendPort, + ); + final R result = await resultPort.first; + Timeline.startSync('$debugLabel: end', flow: Flow.end(flow.id)); + resultPort.close(); + isolate.kill(); + Timeline.finishSync(); + return result; +} + +@immutable +class _IsolateConfiguration<Q, R> { + const _IsolateConfiguration( + this.callback, + this.message, + this.resultPort, + this.debugLabel, + this.flowId, + ); + final ComputeCallback<Q, R> callback; + final Q message; + final SendPort resultPort; + final String debugLabel; + final int flowId; +} + +void _spawn<Q, R>(_IsolateConfiguration<Q, R> configuration) { + R result; + Timeline.timeSync( + '${configuration.debugLabel}', + () { + result = configuration.callback(configuration.message); + }, + flow: Flow.step(configuration.flowId), + ); + Timeline.timeSync( + '${configuration.debugLabel}: returning result', + () { configuration.resultPort.send(result); }, + flow: Flow.step(configuration.flowId), + ); +}
diff --git a/packages/flutter/lib/src/foundation/licenses.dart b/packages/flutter/lib/src/foundation/licenses.dart index db00588..cbacdf4 100644 --- a/packages/flutter/lib/src/foundation/licenses.dart +++ b/packages/flutter/lib/src/foundation/licenses.dart
@@ -54,7 +54,7 @@ } enum _LicenseEntryWithLineBreaksParserState { - beforeParagraph, inParagraph + beforeParagraph, inParagraph, } /// Variant of [LicenseEntry] for licenses that separate paragraphs with blank @@ -63,10 +63,13 @@ /// unless they start with the same number of spaces as the previous line, in /// which case it's assumed they are a continuation of an indented paragraph. /// +/// ## Sample code +/// /// For example, the BSD license in this format could be encoded as follows: /// /// ```dart -/// LicenseRegistry.addLicense(() { +/// void initMyLibrary() { +/// LicenseRegistry.addLicense(() async* { /// yield new LicenseEntryWithLineBreaks(<String>['my_library'], ''' /// Copyright 2016 The Sample Authors. All rights reserved. /// @@ -95,11 +98,20 @@ /// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT /// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE /// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.'''); -/// } +/// }); +/// } /// ``` /// /// This would result in a license with six [paragraphs], the third, fourth, and /// fifth being indented one level. +/// +/// ## Performance considerations +/// +/// Computing the paragraphs is relatively expensive. Doing the work for one +/// license per frame is reasonable; doing more at the same time is ill-advised. +/// Consider doing all the work at once using [compute] to move the work to +/// another thread, or spreading the work across multiple frames using +/// [scheduleTask]. class LicenseEntryWithLineBreaks extends LicenseEntry { /// Create a license entry for a license whose text is hard-wrapped within /// paragraphs and has paragraph breaks denoted by blank lines or with @@ -166,8 +178,9 @@ break; case '\n': case '\f': - if (lines.isNotEmpty) + if (lines.isNotEmpty) { yield getParagraph(); + } lastLineIndent = 0; currentLineIndent = 0; currentParagraphIndentation = null; @@ -227,8 +240,9 @@ } switch (state) { case _LicenseEntryWithLineBreaksParserState.beforeParagraph: - if (lines.isNotEmpty) + if (lines.isNotEmpty) { yield getParagraph(); + } break; case _LicenseEntryWithLineBreaksParserState.inParagraph: addLine(); @@ -283,8 +297,7 @@ /// Returns the licenses that have been registered. /// - /// Because generating the list of licenses is expensive, this function should - /// only be called once. + /// Generating the list of licenses is expensive. static Stream<LicenseEntry> get licenses async* { if (_collectors == null) return;
diff --git a/packages/flutter/lib/src/material/about.dart b/packages/flutter/lib/src/material/about.dart index caffabc..3e1bbb3 100644 --- a/packages/flutter/lib/src/material/about.dart +++ b/packages/flutter/lib/src/material/about.dart
@@ -3,10 +3,12 @@ // found in the LICENSE file. import 'dart:async'; +import 'dart:developer' show Timeline, Flow; import 'dart:io' show Platform; -import 'package:flutter/widgets.dart'; import 'package:flutter/foundation.dart'; +import 'package:flutter/scheduler.dart'; +import 'package:flutter/widgets.dart' hide Flow; import 'app_bar.dart'; import 'debug.dart'; @@ -364,7 +366,6 @@ } class _LicensePageState extends State<LicensePage> { - @override void initState() { super.initState(); @@ -375,9 +376,19 @@ bool _loaded = false; Future<Null> _initLicenses() async { + final Flow flow = Flow.begin(); + Timeline.timeSync('_initLicenses()', () { }, flow: flow); await for (LicenseEntry license in LicenseRegistry.licenses) { if (!mounted) return; + Timeline.timeSync('_initLicenses()', () { }, flow: Flow.step(flow.id)); + final List<LicenseParagraph> paragraphs = + await SchedulerBinding.instance.scheduleTask<List<LicenseParagraph>>( + () => license.paragraphs.toList(), + Priority.animation, + debugLabel: 'License', + flow: flow, + ); setState(() { _licenses.add(const Padding( padding: const EdgeInsets.symmetric(vertical: 18.0), @@ -396,7 +407,7 @@ textAlign: TextAlign.center ) )); - for (LicenseParagraph paragraph in license.paragraphs) { + for (LicenseParagraph paragraph in paragraphs) { if (paragraph.indent == LicenseParagraph.centeredIndent) { _licenses.add(new Padding( padding: const EdgeInsets.only(top: 16.0), @@ -419,6 +430,7 @@ setState(() { _loaded = true; }); + Timeline.timeSync('Build scheduled', () { }, flow: Flow.end(flow.id)); } @override @@ -458,7 +470,6 @@ child: new Scrollbar( child: new ListView( padding: const EdgeInsets.symmetric(horizontal: 8.0, vertical: 12.0), - shrinkWrap: true, children: contents, ), ),
diff --git a/packages/flutter/lib/src/scheduler/binding.dart b/packages/flutter/lib/src/scheduler/binding.dart index 5ff81b1..a9bbac4 100644 --- a/packages/flutter/lib/src/scheduler/binding.dart +++ b/packages/flutter/lib/src/scheduler/binding.dart
@@ -6,7 +6,7 @@ import 'dart:collection'; import 'dart:developer'; import 'dart:ui' as ui show window; -import 'dart:ui' show AppLifecycleState, VoidCallback; +import 'dart:ui' show AppLifecycleState; import 'package:collection/collection.dart' show PriorityQueue, HeapPriorityQueue; import 'package:flutter/foundation.dart'; @@ -33,14 +33,20 @@ _timeDilation = value; } -/// A frame-related callback from the scheduler. +/// Signature for frame-related callbacks from the scheduler. /// -/// The timeStamp is the number of milliseconds since the beginning of the +/// The `timeStamp` is the number of milliseconds since the beginning of the /// scheduler's epoch. Use timeStamp to determine how far to advance animation /// timelines so that all the animations in the system are synchronized to a /// common time base. typedef void FrameCallback(Duration timeStamp); +/// Signature for [Scheduler.scheduleTask] callbacks. +/// +/// The type argument `T` is the task's return value. Consider [void] if the +/// task does not return a value. +typedef T TaskCallback<T>(); + /// Signature for the [SchedulerBinding.schedulingStrategy] callback. Called /// whenever the system needs to decide whether a task at a given /// priority needs to be run. @@ -51,17 +57,32 @@ /// See also [defaultSchedulingStrategy]. typedef bool SchedulingStrategy({ int priority, SchedulerBinding scheduler }); -class _TaskEntry { - _TaskEntry(this.task, this.priority) { +class _TaskEntry<T> { + _TaskEntry(this.task, this.priority, this.debugLabel, this.flow) { + // ignore: prefer_asserts_in_initializer_lists assert(() { debugStack = StackTrace.current; return true; }()); + completer = new Completer<T>(); } - final VoidCallback task; + final TaskCallback<T> task; final int priority; + final String debugLabel; + final Flow flow; StackTrace debugStack; + Completer<T> completer; + + void run() { + Timeline.timeSync( + debugLabel ?? 'Scheduled Task', + () { + completer.complete(task()); + }, + flow: flow != null ? Flow.step(flow.id) : null, + ); + } } class _FrameCallbackEntry { @@ -253,23 +274,45 @@ /// Defaults to [defaultSchedulingStrategy]. SchedulingStrategy schedulingStrategy = defaultSchedulingStrategy; - static int _taskSorter (_TaskEntry e1, _TaskEntry e2) { + static int _taskSorter (_TaskEntry<dynamic> e1, _TaskEntry<dynamic> e2) { return -e1.priority.compareTo(e2.priority); } - final PriorityQueue<_TaskEntry> _taskQueue = new HeapPriorityQueue<_TaskEntry>(_taskSorter); + final PriorityQueue<_TaskEntry<dynamic>> _taskQueue = new HeapPriorityQueue<_TaskEntry<dynamic>>(_taskSorter); - /// Schedules the given `task` with the given `priority`. + /// Schedules the given `task` with the given `priority` and returns a + /// [Future] that completes to the `task`'s eventual return value. + /// + /// The `debugLabel` and `flow` are used to report the task to the [Timeline], + /// for use when profiling. + /// + /// ## Processing model /// /// Tasks will be executed between frames, in priority order, /// excluding tasks that are skipped by the current /// [schedulingStrategy]. Tasks should be short (as in, up to a /// millisecond), so as to not cause the regular frame callbacks to /// get delayed. - void scheduleTask(VoidCallback task, Priority priority) { + /// + /// If an animation is running, including, for instance, a [ProgressIndicator] + /// indicating that there are pending tasks, then tasks with a priority below + /// [Priority.animation] won't run (at least, not with the + /// [defaultSchedulingStrategy]; this can be configured using + /// [schedulingStrategy]). + Future<T> scheduleTask<T>(TaskCallback<T> task, Priority priority, { + String debugLabel, + Flow flow, + }) { final bool isFirstTask = _taskQueue.isEmpty; - _taskQueue.add(new _TaskEntry(task, priority.value)); + final _TaskEntry<T> entry = new _TaskEntry<T>( + task, + priority.value, + debugLabel, + flow, + ); + _taskQueue.add(entry); if (isFirstTask && !locked) _ensureEventLoopCallback(); + return entry.completer.future; } @override @@ -313,10 +356,11 @@ bool handleEventLoopCallback() { if (_taskQueue.isEmpty || locked) return false; - final _TaskEntry entry = _taskQueue.first; + final _TaskEntry<dynamic> entry = _taskQueue.first; if (schedulingStrategy(priority: entry.priority, scheduler: this)) { try { - (_taskQueue.removeFirst().task)(); + _taskQueue.removeFirst(); + entry.run(); } catch (exception, exceptionStack) { StackTrace callbackStack; assert(() {
diff --git a/packages/flutter/lib/src/services/binding.dart b/packages/flutter/lib/src/services/binding.dart index c43af11..f4e1919 100644 --- a/packages/flutter/lib/src/services/binding.dart +++ b/packages/flutter/lib/src/services/binding.dart
@@ -39,22 +39,29 @@ LicenseRegistry.addLicense(_addLicenses); } - static final String _licenseSeparator = '\n' + ('-' * 80) + '\n'; - Stream<LicenseEntry> _addLicenses() async* { final String rawLicenses = await rootBundle.loadString('LICENSE', cache: false); + final List<LicenseEntry> licenses = await compute(_parseLicenses, rawLicenses, debugLabel: 'parseLicenses'); + yield* new Stream<LicenseEntry>.fromIterable(licenses); + } + + // This is run in another isolate created by _addLicenses above. + static List<LicenseEntry> _parseLicenses(String rawLicenses) { + final String _licenseSeparator = '\n' + ('-' * 80) + '\n'; + final List<LicenseEntry> result = <LicenseEntry>[]; final List<String> licenses = rawLicenses.split(_licenseSeparator); for (String license in licenses) { final int split = license.indexOf('\n\n'); if (split >= 0) { - yield new LicenseEntryWithLineBreaks( + result.add(new LicenseEntryWithLineBreaks( license.substring(0, split).split('\n'), license.substring(split + 2) - ); + )); } else { - yield new LicenseEntryWithLineBreaks(const <String>[], license); + result.add(new LicenseEntryWithLineBreaks(const <String>[], license)); } } + return result; } @override
diff --git a/packages/flutter/test/material/about_test.dart b/packages/flutter/test/material/about_test.dart index d922198..2ab1079 100644 --- a/packages/flutter/test/material/about_test.dart +++ b/packages/flutter/test/material/about_test.dart
@@ -76,36 +76,36 @@ }); testWidgets('AboutListTile control test', (WidgetTester tester) async { - final List<String> log = <String>[]; - - Future<Null> licenseFuture; LicenseRegistry.addLicense(() { - log.add('license1'); - licenseFuture = tester.pumpWidget(new Container()); - return new Stream<LicenseEntry>.fromIterable(<LicenseEntry>[]); + return new Stream<LicenseEntry>.fromIterable(<LicenseEntry>[ + new LicenseEntryWithLineBreaks(<String>['AAA'], 'BBB') + ]); }); LicenseRegistry.addLicense(() { - log.add('license2'); return new Stream<LicenseEntry>.fromIterable(<LicenseEntry>[ - new LicenseEntryWithLineBreaks(<String>[ 'Another package '], 'Another license') + new LicenseEntryWithLineBreaks(<String>['Another package'], 'Another license') ]); }); await tester.pumpWidget( new MaterialApp( home: const Center( - child: const LicensePage() + child: const LicensePage(), ), ), ); - expect(licenseFuture, isNotNull); - await licenseFuture; + expect(find.text('AAA'), findsNothing); + expect(find.text('BBB'), findsNothing); + expect(find.text('Another package'), findsNothing); + expect(find.text('Another license'), findsNothing); - // We should not hit an exception here. - await tester.idle(); + await tester.pumpAndSettle(); - expect(log, equals(<String>['license1', 'license2'])); + expect(find.text('AAA'), findsOneWidget); + expect(find.text('BBB'), findsOneWidget); + expect(find.text('Another package'), findsOneWidget); + expect(find.text('Another license'), findsOneWidget); }); }