From 0d526438a5f2eb33a54e5465623da8e7053650a4 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Thu, 9 Jul 2020 15:42:02 -0700 Subject: [PATCH] Performance improvements 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. --- lib/dartdoc.dart | 5 +- lib/src/model/canonicalization.dart | 4 +- lib/src/model/library.dart | 10 +- lib/src/model/model_element.dart | 161 +++++++++++++++------------- lib/src/model/package.dart | 29 ++--- lib/src/model/package_graph.dart | 33 +++--- 6 files changed, 125 insertions(+), 117 deletions(-) diff --git a/lib/dartdoc.dart b/lib/dartdoc.dart index 6a06703b14..0416fa3de0 100644 --- a/lib/dartdoc.dart +++ b/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; diff --git a/lib/src/model/canonicalization.dart b/lib/src/model/canonicalization.dart index 9728e3e644..be33a0468f 100644 --- a/lib/src/model/canonicalization.dart +++ b/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 get locationPieces; - List scoreCanonicalCandidates(List libraries) { - return libraries.map((l) => scoreElementWithLibrary(l)).toList()..sort(); + List scoreCanonicalCandidates(Iterable libraries) { + return libraries.map(scoreElementWithLibrary).toList()..sort(); } ScoredCandidate scoreElementWithLibrary(Library lib) { diff --git a/lib/src/model/library.dart b/lib/src/model/library.dart index 9797e54539..13ac34cdb9 100644 --- a/lib/src/model/library.dart +++ b/lib/src/model/library.dart @@ -628,13 +628,9 @@ class Library extends ModelElement with Categorization, TopLevelContainer { List _allModelElements; Iterable 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 _allCanonicalModelElements; diff --git a/lib/src/model/model_element.dart b/lib/src/model/model_element.dart index 87f21fb844..876a7091ae 100644 --- a/lib/src/model/model_element.dart +++ b/lib/src/model/model_element.dart @@ -473,13 +473,14 @@ abstract class ModelElement extends Canonicalization return _config; } + Set _locationPieces; + @override - Set get locationPieces { - return Set.from(element.location - .toString() - .split(locationSplitter) - .where((s) => s.isNotEmpty)); - } + Set get locationPieces => + _locationPieces ??= Set.from(element.location + .toString() + .split(locationSplitter) + .where((s) => s.isNotEmpty)); Set get features { var allFeatures = {}; @@ -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; diff --git a/lib/src/model/package.dart b/lib/src/model/package.dart index 01ff9d9085..34cb1b2992 100644 --- a/lib/src/model/package.dart +++ b/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 diff --git a/lib/src/model/package_graph.dart b/lib/src/model/package_graph.dart index 6cd54e1209..0266f03f82 100644 --- a/lib/src/model/package_graph.dart +++ b/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 _allCanonicalModelElements; Iterable get allCanonicalModelElements { - return (_allCanonicalModelElements ??= - allLocalModelElements.where((e) => e.isCanonical).toList()); + return _allCanonicalModelElements ??= + allLocalModelElements.where((e) => e.isCanonical).toList(); } String getMacro(String name) {