Performance improvements (#2259)

When documenting the camera Flutter plugin, these changes reduce the time to
document from 52 seconds to 51 seconds.

Much of this speedup likely came from memoizing Package.documentedWhere, which
has a few expensive calls like `linkToRemote` and `linkToUrl`. Additionally
ModelElement.locationPieces is now memoized.

The rest is a few refactorings:

* ModelElement.canonicalLibrary:
    * Refactor most of the complexity of ModelElement.canonicalLibrary into a
      new private method.
    * Don't compute topLevelElement until you need it.
    * Short circuit if `definingLibrary.exportedInLibraries` is null.
    * Reduce nesting
    * Create warning message only if it will be used.
* Prefer for loops over Iterable.forEach.

Author: srawlins

lib/dartdoc.dart

@@ -290,10 +290,10 @@ class Dartdoc {
     var indexJson = path.joinAll([normalOrigin, 'index.json']);
     var foundIndexJson = false;
     for (var f in Directory(normalOrigin).listSync(recursive: true)) {
-      var fullPath = path.normalize(f.path);
       if (f is Directory) {
         continue;
       }
+      var fullPath = path.normalize(f.path);
       if (fullPath.startsWith(staticAssets)) {
         continue;
       }
@@ -331,6 +331,9 @@ class Dartdoc {
     if (!file.existsSync()) {
       return null;
     }
+    // TODO(srawlins): It is possible that instantiating an HtmlParser using
+    // `lowercaseElementName: false` and `lowercaseAttrName: false` may save
+    // time or memory.
     var doc = parse(file.readAsBytesSync());
     var base = doc.querySelector('base');
     String baseHref;

lib/src/model/canonicalization.dart

@@ -15,8 +15,8 @@ abstract class Canonicalization implements Locatable, Documentable {
   /// Pieces of the location, split to remove 'package:' and slashes.
   Set<String> get locationPieces;
 
-  List<ScoredCandidate> scoreCanonicalCandidates(List<Library> libraries) {
-    return libraries.map((l) => scoreElementWithLibrary(l)).toList()..sort();
+  List<ScoredCandidate> scoreCanonicalCandidates(Iterable<Library> libraries) {
+    return libraries.map(scoreElementWithLibrary).toList()..sort();
   }
 
   ScoredCandidate scoreElementWithLibrary(Library lib) {

lib/src/model/library.dart

@@ -628,13 +628,9 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
   List<ModelElement> _allModelElements;
 
   Iterable<ModelElement> get allModelElements {
-    if (_allModelElements == null) {
-      _allModelElements = [];
-      for (var modelElements in modelElementsMap.values) {
-        _allModelElements.addAll(modelElements);
-      }
-    }
-    return _allModelElements;
+    return _allModelElements ??= [
+      for (var modelElements in modelElementsMap.values) ...modelElements,
+    ];
   }
 
   List<ModelElement> _allCanonicalModelElements;

lib/src/model/model_element.dart

@@ -473,13 +473,14 @@ abstract class ModelElement extends Canonicalization
     return _config;
   }
 
+  Set<String> _locationPieces;
+
   @override
-  Set<String> get locationPieces {
-    return Set.from(element.location
-        .toString()
-        .split(locationSplitter)
-        .where((s) => s.isNotEmpty));
-  }
+  Set<String> get locationPieces =>
+      _locationPieces ??= Set.from(element.location
+          .toString()
+          .split(locationSplitter)
+          .where((s) => s.isNotEmpty));
 
   Set<String> get features {
     var allFeatures = <String>{};
@@ -648,81 +649,13 @@ abstract class ModelElement extends Canonicalization
     if (!_canonicalLibraryIsSet) {
       // This is not accurate if we are constructing the Package.
       assert(packageGraph.allLibrariesAdded);
-      // Since we're looking for a library, find the [Element] immediately
-      // contained by a [CompilationUnitElement] in the tree.
-      var topLevelElement = element;
-      while (topLevelElement != null &&
-          topLevelElement.enclosingElement is! LibraryElement &&
-          topLevelElement.enclosingElement is! CompilationUnitElement &&
-          topLevelElement.enclosingElement != null) {
-        topLevelElement = topLevelElement.enclosingElement;
-      }
 
       // Privately named elements can never have a canonical library, so
       // just shortcut them out.
       if (!utils.hasPublicName(element)) {
         _canonicalLibrary = null;
-      } else if (definingLibrary != null &&
-          !packageGraph.localPublicLibraries.contains(definingLibrary)) {
-        var candidateLibraries = definingLibrary.exportedInLibraries
-            ?.where((l) =>
-                l.isPublic &&
-                l.package.documentedWhere != DocumentLocation.missing)
-            ?.toList();
-
-        if (candidateLibraries != null) {
-          candidateLibraries = candidateLibraries.where((l) {
-            var lookup =
-                l.element.exportNamespace.definedNames[topLevelElement?.name];
-            if (lookup is PropertyAccessorElement) {
-              lookup = (lookup as PropertyAccessorElement).variable;
-            }
-            if (topLevelElement == lookup) return true;
-            return false;
-          }).toList();
-
-          // Avoid claiming canonicalization for elements outside of this element's
-          // defining package.
-          // TODO(jcollins-g): Make the else block unconditional.
-          if (candidateLibraries.isNotEmpty &&
-              !candidateLibraries
-                  .any((l) => l.package == definingLibrary.package)) {
-            warn(PackageWarning.reexportedPrivateApiAcrossPackages,
-                message: definingLibrary.package.fullyQualifiedName,
-                referredFrom: candidateLibraries);
-          } else {
-            candidateLibraries
-                .removeWhere((l) => l.package != definingLibrary.package);
-          }
-
-          // Start with our top-level element.
-          var warnable =
-              ModelElement.fromElement(topLevelElement, packageGraph);
-          if (candidateLibraries.length > 1) {
-            // Heuristic scoring to determine which library a human likely
-            // considers this element to be primarily 'from', and therefore,
-            // canonical.  Still warn if the heuristic isn't that confident.
-            var scoredCandidates =
-                warnable.scoreCanonicalCandidates(candidateLibraries);
-            candidateLibraries =
-                scoredCandidates.map((s) => s.library).toList();
-            var secondHighestScore =
-                scoredCandidates[scoredCandidates.length - 2].score;
-            var highestScore = scoredCandidates.last.score;
-            var confidence = highestScore - secondHighestScore;
-            var message =
-                '${candidateLibraries.map((l) => l.name)} -> ${candidateLibraries.last.name} (confidence ${confidence.toStringAsPrecision(4)})';
-
-            if (confidence < config.ambiguousReexportScorerMinConfidence) {
-              warnable.warn(PackageWarning.ambiguousReexport,
-                  message: message,
-                  extendedDebug: scoredCandidates.map((s) => '$s'));
-            }
-          }
-          if (candidateLibraries.isNotEmpty) {
-            _canonicalLibrary = candidateLibraries.last;
-          }
-        }
+      } else if (!packageGraph.localPublicLibraries.contains(definingLibrary)) {
+        _canonicalLibrary = _searchForCanonicalLibrary();
       } else {
         _canonicalLibrary = definingLibrary;
       }
@@ -743,6 +676,79 @@ abstract class ModelElement extends Canonicalization
     return _canonicalLibrary;
   }
 
+  Library _searchForCanonicalLibrary() {
+    var thisAndExported = definingLibrary.exportedInLibraries;
+
+    if (thisAndExported == null) {
+      return null;
+    }
+
+    // Since we're looking for a library, find the [Element] immediately
+    // contained by a [CompilationUnitElement] in the tree.
+    var topLevelElement = element;
+    while (topLevelElement != null &&
+        topLevelElement.enclosingElement is! LibraryElement &&
+        topLevelElement.enclosingElement is! CompilationUnitElement &&
+        topLevelElement.enclosingElement != null) {
+      topLevelElement = topLevelElement.enclosingElement;
+    }
+
+    var candidateLibraries = thisAndExported
+        .where((l) =>
+            l.isPublic && l.package.documentedWhere != DocumentLocation.missing)
+        .where((l) {
+      var lookup =
+          l.element.exportNamespace.definedNames[topLevelElement?.name];
+      if (lookup is PropertyAccessorElement) {
+        lookup = (lookup as PropertyAccessorElement).variable;
+      }
+      return topLevelElement == lookup;
+    }).toList();
+
+    // Avoid claiming canonicalization for elements outside of this element's
+    // defining package.
+    // TODO(jcollins-g): Make the else block unconditional.
+    if (candidateLibraries.isNotEmpty &&
+        !candidateLibraries.any((l) => l.package == definingLibrary.package)) {
+      warn(PackageWarning.reexportedPrivateApiAcrossPackages,
+          message: definingLibrary.package.fullyQualifiedName,
+          referredFrom: candidateLibraries);
+    } else {
+      candidateLibraries
+          .removeWhere((l) => l.package != definingLibrary.package);
+    }
+
+    if (candidateLibraries.isEmpty) {
+      return null;
+    }
+    if (candidateLibraries.length == 1) {
+      return candidateLibraries.single;
+    }
+
+    // Start with our top-level element.
+    var warnable = ModelElement.fromElement(topLevelElement, packageGraph);
+    // Heuristic scoring to determine which library a human likely
+    // considers this element to be primarily 'from', and therefore,
+    // canonical.  Still warn if the heuristic isn't that confident.
+    var scoredCandidates =
+        warnable.scoreCanonicalCandidates(candidateLibraries);
+    candidateLibraries = scoredCandidates.map((s) => s.library).toList();
+    var secondHighestScore =
+        scoredCandidates[scoredCandidates.length - 2].score;
+    var highestScore = scoredCandidates.last.score;
+    var confidence = highestScore - secondHighestScore;
+
+    if (confidence < config.ambiguousReexportScorerMinConfidence) {
+      var libraryNames = candidateLibraries.map((l) => l.name);
+      var message = '$libraryNames -> ${candidateLibraries.last.name} '
+          '(confidence ${confidence.toStringAsPrecision(4)})';
+      warnable.warn(PackageWarning.ambiguousReexport,
+          message: message, extendedDebug: scoredCandidates.map((s) => '$s'));
+    }
+
+    return candidateLibraries.last;
+  }
+
   @override
   bool get isCanonical {
     if (!isPublic) return false;
@@ -1072,6 +1078,9 @@ abstract class ModelElement extends Canonicalization
 
   String computeDocumentationComment() => element.documentationComment;
 
+  /// The documentation comment on the Element may be null, so memoization
+  /// cannot rely on the null-ness of [_documentationComment], it must be
+  /// more explicit.
   bool _documentationCommentComputed = false;
   String _documentationComment;
 

lib/src/model/package.dart

@@ -171,22 +171,27 @@ class Package extends LibraryContainer
     return _isLocal;
   }
 
+  /* late */ DocumentLocation _documentedWhere;
+
   DocumentLocation get documentedWhere {
-    if (isLocal) {
-      if (isPublic) {
-        return DocumentLocation.local;
-      } else {
-        // Possible if excludes result in a "documented" package not having
-        // any actual documentation.
-        return DocumentLocation.missing;
-      }
-    } else {
-      if (config.linkToRemote && config.linkToUrl.isNotEmpty && isPublic) {
-        return DocumentLocation.remote;
+    if (_documentedWhere == null) {
+      if (isLocal) {
+        if (isPublic) {
+          _documentedWhere = DocumentLocation.local;
+        } else {
+          // Possible if excludes result in a "documented" package not having
+          // any actual documentation.
+          _documentedWhere = DocumentLocation.missing;
+        }
       } else {
-        return DocumentLocation.missing;
+        if (config.linkToRemote && config.linkToUrl.isNotEmpty && isPublic) {
+          _documentedWhere = DocumentLocation.remote;
+        } else {
+          _documentedWhere = DocumentLocation.missing;
+        }
       }
     }
+    return _documentedWhere;
   }
 
   @override

lib/src/model/package_graph.dart

@@ -68,24 +68,23 @@ class PackageGraph {
     specialClasses = SpecialClasses();
     // Go through docs of every ModelElement in package to pre-build the macros
     // index.  Uses toList() in order to get all the precaching on the stack.
-    var precacheFutures = precacheLocalDocs().toList();
-    for (var f in precacheFutures) {
-      await f;
-    }
+    await Future.wait(precacheLocalDocs());
     _localDocumentationBuilt = true;
 
     // Scan all model elements to insure that interceptor and other special
     // objects are found.
     // After the allModelElements traversal to be sure that all packages
     // are picked up.
-    documentedPackages.toList().forEach((package) {
+    for (var package in documentedPackages) {
       package.libraries.sort((a, b) => compareNatural(a.name, b.name));
-      package.libraries.forEach((library) {
+      for (var library in package.libraries) {
         library.allClasses.forEach(_addToImplementors);
         _extensions.addAll(library.extensions);
-      });
-    });
-    _implementors.values.forEach((l) => l.sort());
+      }
+    }
+    for (var l in _implementors.values) {
+      l.sort();
+    }
     allImplementorsAdded = true;
     allExtensionsAdded = true;
 
@@ -574,18 +573,14 @@ class PackageGraph {
       }
     }
 
-    if (c.mixins.isNotEmpty) {
-      c.mixins.forEach((t) {
-        _checkAndAddClass(t.element, c);
-      });
+    for (var type in c.mixins) {
+      _checkAndAddClass(type.element, c);
     }
     if (c.supertype != null) {
       _checkAndAddClass(c.supertype.element, c);
     }
-    if (c.interfaces.isNotEmpty) {
-      c.interfaces.forEach((t) {
-        _checkAndAddClass(t.element, c);
-      });
+    for (var type in c.interfaces) {
+      _checkAndAddClass(type.element, c);
     }
   }
 
@@ -908,8 +903,8 @@ class PackageGraph {
   List<ModelElement> _allCanonicalModelElements;
 
   Iterable<ModelElement> get allCanonicalModelElements {
-    return (_allCanonicalModelElements ??=
-        allLocalModelElements.where((e) => e.isCanonical).toList());
+    return _allCanonicalModelElements ??=
+        allLocalModelElements.where((e) => e.isCanonical).toList();
   }
 
   String getMacro(String name) {