Performance improvements for documentation generation (#1837)

* Split out documentation comment computation and make it cached

* Precache local documentation only if it is possible a macro template is defined there

* Directory.current is expensive, also get rid of excessive split/joins for strings

* Minor tweaks

* dartfmt

* Stable doesn't like Future<void>

* Fix import for stable

Author: jcollins-g

lib/src/dartdoc_options.dart

@@ -32,6 +32,12 @@ const int _kIntVal = 0;
 const double _kDoubleVal = 0.0;
 const bool _kBoolVal = true;
 
+/// Args are computed relative to the current directory at the time the
+/// program starts.
+final Directory directoryCurrent = Directory.current;
+final String directoryCurrentPath =
+    pathLib.canonicalize(Directory.current.path);
+
 String resolveTildePath(String originalPath) {
   if (originalPath == null || !originalPath.startsWith('~/')) {
     return originalPath;
@@ -585,8 +591,8 @@ abstract class DartdocOption<T> {
   /// corresponding files or directories.
   T valueAt(Directory dir);
 
-  /// Calls [valueAt] with the current working directory.
-  T valueAtCurrent() => valueAt(Directory.current);
+  /// Calls [valueAt] with the working directory at the start of the program.
+  T valueAtCurrent() => valueAt(directoryCurrent);
 
   /// Calls [valueAt] on the directory this element is defined in.
   T valueAtElement(Element element) => valueAt(new Directory(
@@ -945,7 +951,7 @@ abstract class _DartdocFileOption<T> implements DartdocOption<T> {
     return _valueAtFromFiles(dir) ?? defaultsTo;
   }
 
-  Map<String, T> __valueAtFromFiles = new Map();
+  final Map<String, T> __valueAtFromFiles = new Map();
   // The value of this option from files will not change unless files are
   // modified during execution (not allowed in Dartdoc).
   T _valueAtFromFiles(Directory dir) {
@@ -1050,8 +1056,8 @@ abstract class _DartdocFileOption<T> implements DartdocOption<T> {
   _YamlFileData _yamlAtDirectory(Directory dir) {
     List<String> canonicalPaths = [pathLib.canonicalize(dir.path)];
     if (!_yamlAtCanonicalPathCache.containsKey(canonicalPaths.first)) {
-      _YamlFileData yamlData = new _YamlFileData(
-          new Map(), pathLib.canonicalize(Directory.current.path));
+      _YamlFileData yamlData =
+          new _YamlFileData(new Map(), directoryCurrentPath);
       if (dir.existsSync()) {
         File dartdocOptionsFile;
 
@@ -1126,7 +1132,7 @@ abstract class _DartdocArgOption<T> implements DartdocOption<T> {
   }
 
   /// Generates an _OptionValueWithContext using the value of the argument from
-  /// the [argParser] and the working directory from [Directory.current].
+  /// the [argParser] and the working directory from [directoryCurrent].
   ///
   /// Throws [UnsupportedError] if [T] is not a supported type.
   _OptionValueWithContext _valueAtFromArgsWithContext() {
@@ -1157,7 +1163,7 @@ abstract class _DartdocArgOption<T> implements DartdocOption<T> {
     } else {
       throw UnsupportedError('Type ${T} is not supported');
     }
-    return new _OptionValueWithContext(retval, Directory.current.path);
+    return new _OptionValueWithContext(retval, directoryCurrentPath);
   }
 
   /// The name of this option as a command line argument.
@@ -1234,8 +1240,8 @@ class DartdocOptionContext {
   /// the inputDir flag to determine the context.
   DartdocOptionContext(this.optionSet, FileSystemEntity entity) {
     if (entity == null) {
-      String inputDir = optionSet['inputDir'].valueAt(Directory.current) ??
-          Directory.current.path;
+      String inputDir = optionSet['inputDir'].valueAt(directoryCurrent) ??
+          directoryCurrentPath;
       context = new Directory(inputDir);
     } else {
       context = new Directory(pathLib
@@ -1404,7 +1410,7 @@ Future<List<DartdocOption>> createDartdocOptions() async {
     new DartdocOptionArgOnly<bool>('injectHtml', false,
         help: 'Allow the use of the {@inject-html} directive to inject raw '
             'HTML into dartdoc output.'),
-    new DartdocOptionArgOnly<String>('input', Directory.current.path,
+    new DartdocOptionArgOnly<String>('input', directoryCurrentPath,
         isDir: true, help: 'Path to source directory', mustExist: true),
     new DartdocOptionSyntheticOnly<String>('inputDir',
         (DartdocSyntheticOption<String> option, Directory dir) {

lib/src/markdown_processor.dart

@@ -198,8 +198,9 @@ MatchingLinkResult _getMatchingLinkElement(
   // Try expensive not-scoped lookup.
   if (refModelElement == null && element is ModelElement) {
     Class preferredClass = _getPreferredClass(element);
-    refModelElement =
-        new _MarkdownCommentReference(codeRef, element, commentRefs, preferredClass).computeReferredElement();
+    refModelElement = new _MarkdownCommentReference(
+            codeRef, element, commentRefs, preferredClass)
+        .computeReferredElement();
   }
 
   // Did not find it anywhere.
@@ -269,32 +270,40 @@ Element _getRefElementFromCommentRefs(
   return null;
 }
 
-
 /// Represents a single comment reference.
 class _MarkdownCommentReference {
   /// The code reference text.
   final String codeRef;
+
   /// The element containing the code reference.
   final Warnable element;
+
   /// A list of [CommentReference]s from the analyzer.
   final List<CommentReference> commentRefs;
+
   /// Disambiguate inheritance with this class.
   final Class preferredClass;
+
   /// Current results.  Input/output of all _find and _reduce methods.
   Set<ModelElement> results;
+
   /// codeRef with any leading constructor string, stripped.
   String codeRefChomped;
+
   /// Library associated with this element.
   Library library;
+
   /// PackageGraph associated with this element.
   PackageGraph packageGraph;
 
-  _MarkdownCommentReference(this.codeRef, this.element, this.commentRefs, this.preferredClass) {
+  _MarkdownCommentReference(
+      this.codeRef, this.element, this.commentRefs, this.preferredClass) {
     assert(element != null);
     assert(element.packageGraph.allLibrariesAdded);
 
     codeRefChomped = codeRef.replaceFirst(isConstructor, '');
-    library = element is ModelElement ? (element as ModelElement).library : null;
+    library =
+        element is ModelElement ? (element as ModelElement).library : null;
     packageGraph = library.packageGraph;
   }
 
@@ -334,7 +343,8 @@ class _MarkdownCommentReference {
       // This could conceivably be a reference to an enum member.  They don't show up in allModelElements.
       _findEnumReferences,
       // Use the analyzer to resolve a comment reference.
-      _findAnalyzerReferences]) {
+      _findAnalyzerReferences
+    ]) {
       findMethod();
       // Remove any "null" objects after each step of trying to add to results.
       // TODO(jcollins-g): Eliminate all situations where nulls can be added
@@ -382,13 +392,20 @@ class _MarkdownCommentReference {
       if (!results.every((r) => r is Parameter)) {
         element.warn(PackageWarning.ambiguousDocReference,
             message:
-            "[$codeRef] => ${results.map((r) => "'${r.fullyQualifiedName}'").join(", ")}");
+                "[$codeRef] => ${results.map((r) => "'${r.fullyQualifiedName}'").join(", ")}");
       }
       result = results.first;
     }
     return result;
   }
 
+  List<String> _codeRefParts;
+  List<String> get codeRefParts => _codeRefParts ??= codeRef.split('.');
+
+  List<String> _codeRefChompedParts;
+  List<String> get codeRefChompedParts =>
+      _codeRefChompedParts ??= codeRefChomped.split('.');
+
   /// Returns true if this is a constructor we should consider due to its
   /// name and the code reference, or if this isn't a constructor.  False
   /// otherwise.
@@ -397,7 +414,6 @@ class _MarkdownCommentReference {
     if (modelElement is! Constructor) return true;
     if (codeRef.contains(isConstructor)) return true;
     Constructor aConstructor = modelElement;
-    List<String> codeRefParts = codeRef.split('.');
     if (codeRefParts.length > 1) {
       // Pick the last two parts, in case a specific library was part of the
       // codeRef.
@@ -434,18 +450,20 @@ class _MarkdownCommentReference {
 
   void _reducePreferLibrariesInLocalImportExportGraph() {
     if (results.any(
-            (r) => library.packageImportedExportedLibraries.contains(r.library))) {
+        (r) => library.packageImportedExportedLibraries.contains(r.library))) {
       results.removeWhere(
-              (r) => !library.packageImportedExportedLibraries.contains(r.library));
+          (r) => !library.packageImportedExportedLibraries.contains(r.library));
     }
   }
 
   void _reducePreferResultsAccessibleInSameLibrary() {
     // TODO(jcollins-g): we could have saved ourselves some work by using the analyzer
     //                   to search the namespace, somehow.  Do that instead.
-    if (element is ModelElement && results.any((r) => r.element.isAccessibleIn((element as ModelElement).library.element))) {
-      results.removeWhere(
-              (r) => !r.element.isAccessibleIn((element as ModelElement).library.element));
+    if (element is ModelElement &&
+        results.any((r) => r.element
+            .isAccessibleIn((element as ModelElement).library.element))) {
+      results.removeWhere((r) =>
+          !r.element.isAccessibleIn((element as ModelElement).library.element));
     }
   }
 
@@ -464,53 +482,55 @@ class _MarkdownCommentReference {
   void _findTypeParameters() {
     if (element is TypeParameters) {
       results.addAll((element as TypeParameters).typeParameters.where((p) =>
-      p.name == codeRefChomped || codeRefChomped.startsWith("${p.name}.")));
+          p.name == codeRefChomped || codeRefChomped.startsWith("${p.name}.")));
     }
   }
 
   void _findParameters() {
     if (element is ModelElement) {
       results.addAll((element as ModelElement).allParameters.where((p) =>
-      p.name == codeRefChomped || codeRefChomped.startsWith("${p.name}.")));
+          p.name == codeRefChomped || codeRefChomped.startsWith("${p.name}.")));
     }
   }
 
   void _findWithoutLeadingIgnoreStuff() {
     if (codeRef.contains(leadingIgnoreStuff)) {
       String newCodeRef = codeRef.replaceFirst(leadingIgnoreStuff, '');
       results.add(new _MarkdownCommentReference(
-          newCodeRef, element, commentRefs, preferredClass).computeReferredElement());
+              newCodeRef, element, commentRefs, preferredClass)
+          .computeReferredElement());
     }
   }
 
   void _findWithoutTrailingIgnoreStuff() {
     if (codeRef.contains(trailingIgnoreStuff)) {
       String newCodeRef = codeRef.replaceFirst(trailingIgnoreStuff, '');
       results.add(new _MarkdownCommentReference(
-          newCodeRef, element, commentRefs, preferredClass).computeReferredElement());
+              newCodeRef, element, commentRefs, preferredClass)
+          .computeReferredElement());
     }
   }
 
   void _findWithoutOperatorPrefix() {
     if (codeRef.startsWith(operatorPrefix)) {
       String newCodeRef = codeRef.replaceFirst(operatorPrefix, '');
       results.add(new _MarkdownCommentReference(
-          newCodeRef, element, commentRefs, preferredClass).computeReferredElement());
+              newCodeRef, element, commentRefs, preferredClass)
+          .computeReferredElement());
     }
   }
 
   void _findEnumReferences() {
     // TODO(jcollins-g): Put enum members in allModelElements with useful hrefs without blowing up other assumptions about what that means.
     // TODO(jcollins-g): This doesn't provide good warnings if an enum and class have the same name in different libraries in the same package.  Fix that.
-    List<String> codeRefChompedParts = codeRefChomped.split('.');
     if (codeRefChompedParts.length >= 2) {
       String maybeEnumName = codeRefChompedParts
           .sublist(0, codeRefChompedParts.length - 1)
           .join('.');
       String maybeEnumMember = codeRefChompedParts.last;
       if (packageGraph.findRefElementCache.containsKey(maybeEnumName)) {
         for (final modelElement
-        in packageGraph.findRefElementCache[maybeEnumName]) {
+            in packageGraph.findRefElementCache[maybeEnumName]) {
           if (modelElement is Enum) {
             if (modelElement.constants.any((e) => e.name == maybeEnumMember)) {
               results.add(modelElement);
@@ -525,7 +545,7 @@ class _MarkdownCommentReference {
   void _findGlobalWithinRefElementCache() {
     if (packageGraph.findRefElementCache.containsKey(codeRefChomped)) {
       for (final modelElement
-      in packageGraph.findRefElementCache[codeRefChomped]) {
+          in packageGraph.findRefElementCache[codeRefChomped]) {
         if (codeRefChomped == modelElement.fullyQualifiedNameWithoutLibrary ||
             (modelElement is Library &&
                 codeRefChomped == modelElement.fullyQualifiedName)) {
@@ -552,10 +572,10 @@ class _MarkdownCommentReference {
     // We now need the ref element cache to keep from repeatedly searching [Package.allModelElements].
     // But if not, look for a fully qualified match.  (That only makes sense
     // if the codeRef might be qualified, and contains periods.)
-    if (
-    codeRefChomped.contains('.') &&
+    if (codeRefChomped.contains('.') &&
         packageGraph.findRefElementCache.containsKey(codeRefChomped)) {
-      for (final ModelElement modelElement in packageGraph.findRefElementCache[codeRefChomped]) {
+      for (final ModelElement modelElement
+          in packageGraph.findRefElementCache[codeRefChomped]) {
         if (!_ConsiderIfConstructor(modelElement)) continue;
         // For fully qualified matches, the original preferredClass passed
         // might make no sense.  Instead, use the enclosing class from the
@@ -576,10 +596,12 @@ class _MarkdownCommentReference {
     List<Class> tryClasses = [preferredClass];
     Class realClass = tryClasses.first;
     if (element is Inheritable) {
-      Inheritable overriddenElement = (element as Inheritable).overriddenElement;
+      Inheritable overriddenElement =
+          (element as Inheritable).overriddenElement;
       while (overriddenElement != null) {
         tryClasses.add(
-            ((element as Inheritable).overriddenElement as EnclosedElement).enclosingElement);
+            ((element as Inheritable).overriddenElement as EnclosedElement)
+                .enclosingElement);
         overriddenElement = overriddenElement.overriddenElement;
       }
     }
@@ -594,7 +616,7 @@ class _MarkdownCommentReference {
 
     if (results.isEmpty && realClass != null) {
       for (Class superClass
-      in realClass.publicSuperChain.map((et) => et.element as Class)) {
+          in realClass.publicSuperChain.map((et) => et.element as Class)) {
         if (!tryClasses.contains(superClass)) {
           _getResultsForClass(superClass);
         }
@@ -613,7 +635,8 @@ class _MarkdownCommentReference {
       if (refModelElement is Accessor) {
         refModelElement = (refModelElement as Accessor).enclosingCombo;
       }
-      refModelElement = refModelElement.canonicalModelElement ?? refModelElement;
+      refModelElement =
+          refModelElement.canonicalModelElement ?? refModelElement;
       results.add(refModelElement);
     }
   }
@@ -626,13 +649,14 @@ class _MarkdownCommentReference {
     if ((tryClass.modelType.typeArguments.map((e) => e.name))
         .contains(codeRefChomped)) {
       results.add((tryClass.modelType.typeArguments.firstWhere(
-              (e) => e.name == codeRefChomped && e is DefinedElementType)
-      as DefinedElementType)
+                  (e) => e.name == codeRefChomped && e is DefinedElementType)
+              as DefinedElementType)
           .element);
     } else {
       // People like to use 'this' in docrefs too.
       if (codeRef == 'this') {
-        results.add(packageGraph.findCanonicalModelElementFor(tryClass.element));
+        results
+            .add(packageGraph.findCanonicalModelElementFor(tryClass.element));
       } else {
         // TODO(jcollins-g): get rid of reimplementation of identifier resolution
         //                   or integrate into ModelElement in a simpler way.
@@ -643,7 +667,6 @@ class _MarkdownCommentReference {
         // TODO(jcollins-g): This makes our caller ~O(n^2) vs length of superChain.
         //                   Fortunately superChains are short, but optimize this if it matters.
         superChain.addAll(tryClass.superChain.map((t) => t.element as Class));
-        List<String> codeRefParts = codeRefChomped.split('.');
         for (final c in superChain) {
           // TODO(jcollins-g): add a hash-map-enabled lookup function to Class?
           for (final modelElement in c.allModelElements) {
@@ -667,17 +690,19 @@ class _MarkdownCommentReference {
             // TODO(jcollins-g): Fix partial qualifications in _findRefElementInLibrary so it can tell
             // when it is referenced from a non-documented element?
             // TODO(jcollins-g): We could probably check this early.
-            if (codeRefParts.first == c.name && codeRefParts.last == namePart) {
+            if (codeRefChompedParts.first == c.name &&
+                codeRefChompedParts.last == namePart) {
               results.add(packageGraph.findCanonicalModelElementFor(
                   modelElement.element,
                   preferredClass: tryClass));
               continue;
             }
             if (modelElement is Constructor) {
               // Constructor names don't include the class, so we might miss them in the above search.
-              if (codeRefParts.length > 1) {
-                String codeRefClass = codeRefParts[codeRefParts.length - 2];
-                String codeRefConstructor = codeRefParts.last;
+              if (codeRefChompedParts.length > 1) {
+                String codeRefClass =
+                    codeRefChompedParts[codeRefChompedParts.length - 2];
+                String codeRefConstructor = codeRefChompedParts.last;
                 if (codeRefClass == c.name &&
                     codeRefConstructor ==
                         modelElement.fullyQualifiedName.split('.').last) {