[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

dev/bots/analyze-sample-code.dart

@@ -37,11 +37,16 @@ class Section {
   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 @@ Future<Null> main() async {
   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 @@ Future<Null> main() async {
         }
       }
     }
-    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 @@ Future<Null> main() async {
       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 @@ dependencies:
         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 @@ dependencies:
         }
       } else {
         print('?? $error');
+        keepMain = true;
         errorCount += 1;
       }
     }
@@ -222,6 +235,13 @@ dependencies:
   } 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);
     }

packages/flutter/lib/foundation.dart

@@ -39,6 +39,7 @@ export 'src/foundation/change_notifier.dart';
 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';

packages/flutter/lib/src/foundation/isolates.dart

+// 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),
+  );
+}

packages/flutter/lib/src/foundation/licenses.dart

@@ -54,7 +54,7 @@ abstract class LicenseEntry {
 }
 
 enum _LicenseEntryWithLineBreaksParserState {
-  beforeParagraph, inParagraph
+  beforeParagraph, inParagraph,
 }
 
 /// Variant of [LicenseEntry] for licenses that separate paragraphs with blank
@@ -63,10 +63,13 @@ enum _LicenseEntryWithLineBreaksParserState {
 /// 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 @@ enum _LicenseEntryWithLineBreaksParserState {
 /// 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 @@ class LicenseEntryWithLineBreaks extends LicenseEntry {
               break;
             case '\n':
             case '\f':
-              if (lines.isNotEmpty)
+              if (lines.isNotEmpty) {
                 yield getParagraph();
+              }
               lastLineIndent = 0;
               currentLineIndent = 0;
               currentParagraphIndentation = null;
@@ -227,8 +240,9 @@ class LicenseEntryWithLineBreaks extends LicenseEntry {
     }
     switch (state) {
       case _LicenseEntryWithLineBreaksParserState.beforeParagraph:
-        if (lines.isNotEmpty)
+        if (lines.isNotEmpty) {
           yield getParagraph();
+        }
         break;
       case _LicenseEntryWithLineBreaksParserState.inParagraph:
         addLine();
@@ -283,8 +297,7 @@ class LicenseRegistry {
 
   /// 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;

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 LicensePage extends StatefulWidget {
 }
 
 class _LicensePageState extends State<LicensePage> {
-
   @override
   void initState() {
     super.initState();
@@ -375,9 +376,19 @@ class _LicensePageState extends State<LicensePage> {
   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 @@ class _LicensePageState extends State<LicensePage> {
             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 @@ class _LicensePageState extends State<LicensePage> {
     setState(() {
       _loaded = true;
     });
+    Timeline.timeSync('Build scheduled', () { }, flow: Flow.end(flow.id));
   }
 
   @override
@@ -458,7 +470,6 @@ class _LicensePageState extends State<LicensePage> {
           child: new Scrollbar(
             child: new ListView(
               padding: const EdgeInsets.symmetric(horizontal: 8.0, vertical: 12.0),
-              shrinkWrap: true,
               children: contents,
             ),
           ),

packages/flutter/lib/src/scheduler/binding.dart

@@ -6,7 +6,7 @@ import 'dart:async';
 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 @@ set timeDilation(double value) {
   _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 @@ typedef void FrameCallback(Duration timeStamp);
 /// 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 @@ abstract class SchedulerBinding extends BindingBase with ServicesBinding {
   /// 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 @@ abstract class SchedulerBinding extends BindingBase with ServicesBinding {
   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(() {

packages/flutter/lib/src/services/binding.dart

@@ -39,22 +39,29 @@ abstract class ServicesBinding extends BindingBase {
     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

packages/flutter/test/material/about_test.dart

@@ -76,36 +76,36 @@ void main() {
   });
 
   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);
   });
 }