From b97f26f71f8078c3a2c4efb954c21733165ba051 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Wed, 17 Apr 2024 17:14:04 -0700 Subject: [PATCH] Add a per-importer cache for loads that aren't cacheable en masse (#2219) Inspired by comments on #2215 --- lib/src/async_import_cache.dart | 93 ++++++++++++++++++++------------- lib/src/import_cache.dart | 93 ++++++++++++++++++++------------- lib/src/util/map.dart | 6 +++ lib/src/util/option.dart | 12 +++++ 4 files changed, 133 insertions(+), 71 deletions(-) create mode 100644 lib/src/util/option.dart diff --git a/lib/src/async_import_cache.dart b/lib/src/async_import_cache.dart index 8ddc66f3d..6d6e4fa8c 100644 --- a/lib/src/async_import_cache.dart +++ b/lib/src/async_import_cache.dart @@ -16,6 +16,7 @@ import 'importer/no_op.dart'; import 'importer/utils.dart'; import 'io.dart'; import 'logger.dart'; +import 'util/map.dart'; import 'util/nullable.dart'; import 'utils.dart'; @@ -44,30 +45,28 @@ final class AsyncImportCache { /// The `forImport` in each key is true when this canonicalization is for an /// `@import` rule. Otherwise, it's for a `@use` or `@forward` rule. /// - /// This cache isn't used for relative imports, because they depend on the - /// specific base importer. That's stored separately in - /// [_relativeCanonicalizeCache]. + /// This cache covers loads that go through the entire chain of [_importers], + /// but it doesn't cover individual loads or loads in which any importer + /// accesses `containingUrl`. See also [_perImporterCanonicalizeCache]. final _canonicalizeCache = <(Uri, {bool forImport}), AsyncCanonicalizeResult?>{}; - /// The canonicalized URLs for each non-canonical URL that's resolved using a - /// relative importer. + /// Like [_canonicalizeCache] but also includes the specific importer in the + /// key. /// - /// The map's keys have four parts: + /// This is used to cache both relative imports from the base importer and + /// individual importer results in the case where some other component of the + /// importer chain isn't cacheable. + final _perImporterCanonicalizeCache = + <(AsyncImporter, Uri, {bool forImport}), AsyncCanonicalizeResult?>{}; + + /// A map from the keys in [_perImporterCanonicalizeCache] that are generated + /// for relative URL loads agains the base importer to the original relative + /// URLs what were loaded. /// - /// 1. The URL passed to [canonicalize] (the same as in [_canonicalizeCache]). - /// 2. Whether the canonicalization is for an `@import` rule. - /// 3. The `baseImporter` passed to [canonicalize]. - /// 4. The `baseUrl` passed to [canonicalize]. - /// - /// The map's values are the same as the return value of [canonicalize]. - final _relativeCanonicalizeCache = <( - Uri, { - bool forImport, - AsyncImporter baseImporter, - Uri? baseUrl - }), - AsyncCanonicalizeResult?>{}; + /// This is used to invalidate the cache when files are changed. + final _nonCanonicalRelativeUrls = + <(AsyncImporter, Uri, {bool forImport}), Uri>{}; /// The parsed stylesheets for each canonicalized import URL. final _importCache = {}; @@ -155,18 +154,17 @@ final class AsyncImportCache { } if (baseImporter != null && url.scheme == '') { - var relativeResult = await putIfAbsentAsync(_relativeCanonicalizeCache, ( - url, - forImport: forImport, - baseImporter: baseImporter, - baseUrl: baseUrl - ), () async { - var (result, cacheable) = await _canonicalize( - baseImporter, baseUrl?.resolveUri(url) ?? url, baseUrl, forImport); + var resolvedUrl = baseUrl?.resolveUri(url) ?? url; + var key = (baseImporter, resolvedUrl, forImport: forImport); + var relativeResult = + await putIfAbsentAsync(_perImporterCanonicalizeCache, key, () async { + var (result, cacheable) = + await _canonicalize(baseImporter, resolvedUrl, baseUrl, forImport); assert( cacheable, "Relative loads should always be cacheable because they never " "provide access to the containing URL."); + if (baseUrl != null) _nonCanonicalRelativeUrls[key] = url; return result; }); if (relativeResult != null) return relativeResult; @@ -182,17 +180,41 @@ final class AsyncImportCache { // `canonicalize()` calls we've attempted are cacheable. Only if they are do // we store the result in the cache. var cacheable = true; - for (var importer in _importers) { + for (var i = 0; i < _importers.length; i++) { + var importer = _importers[i]; + var perImporterKey = (importer, url, forImport: forImport); + switch (_perImporterCanonicalizeCache.getOption(perImporterKey)) { + case (var result?,): + return result; + case (null,): + continue; + } + switch (await _canonicalize(importer, url, baseUrl, forImport)) { case (var result?, true) when cacheable: _canonicalizeCache[key] = result; return result; - case (var result?, _): - return result; - - case (_, false): - cacheable = false; + case (var result, true) when !cacheable: + _perImporterCanonicalizeCache[perImporterKey] = result; + if (result != null) return result; + + case (var result, false): + if (cacheable) { + // If this is the first uncacheable result, add all previous results + // to the per-importer cache so we don't have to re-run them for + // future uses of this importer. + for (var j = 0; j < i; j++) { + _perImporterCanonicalizeCache[( + _importers[j], + url, + forImport: forImport + )] = null; + } + cacheable = false; + } + + if (result != null) return result; } } @@ -315,7 +337,7 @@ final class AsyncImportCache { Uri sourceMapUrl(Uri canonicalUrl) => _resultsCache[canonicalUrl]?.sourceMapUrl ?? canonicalUrl; - /// Clears the cached canonical version of the given [url]. + /// Clears the cached canonical version of the given non-canonical [url]. /// /// Has no effect if the canonical version of [url] has not been cached. /// @@ -324,7 +346,8 @@ final class AsyncImportCache { void clearCanonicalize(Uri url) { _canonicalizeCache.remove((url, forImport: false)); _canonicalizeCache.remove((url, forImport: true)); - _relativeCanonicalizeCache.removeWhere((key, _) => key.$1 == url); + _perImporterCanonicalizeCache.removeWhere( + (key, _) => key.$2 == url || _nonCanonicalRelativeUrls[key] == url); } /// Clears the cached parse tree for the stylesheet with the given diff --git a/lib/src/import_cache.dart b/lib/src/import_cache.dart index 7e5c941a0..9590b0e5a 100644 --- a/lib/src/import_cache.dart +++ b/lib/src/import_cache.dart @@ -5,7 +5,7 @@ // DO NOT EDIT. This file was generated from async_import_cache.dart. // See tool/grind/synchronize.dart for details. // -// Checksum: 36bc42050cf2eb3a43f36376c4f06c1708eee777 +// Checksum: 4362e28e5cd425786c235d2a6a2bb60539403799 // // ignore_for_file: unused_import @@ -23,6 +23,7 @@ import 'importer/no_op.dart'; import 'importer/utils.dart'; import 'io.dart'; import 'logger.dart'; +import 'util/map.dart'; import 'util/nullable.dart'; import 'utils.dart'; @@ -47,29 +48,26 @@ final class ImportCache { /// The `forImport` in each key is true when this canonicalization is for an /// `@import` rule. Otherwise, it's for a `@use` or `@forward` rule. /// - /// This cache isn't used for relative imports, because they depend on the - /// specific base importer. That's stored separately in - /// [_relativeCanonicalizeCache]. + /// This cache covers loads that go through the entire chain of [_importers], + /// but it doesn't cover individual loads or loads in which any importer + /// accesses `containingUrl`. See also [_perImporterCanonicalizeCache]. final _canonicalizeCache = <(Uri, {bool forImport}), CanonicalizeResult?>{}; - /// The canonicalized URLs for each non-canonical URL that's resolved using a - /// relative importer. + /// Like [_canonicalizeCache] but also includes the specific importer in the + /// key. /// - /// The map's keys have four parts: + /// This is used to cache both relative imports from the base importer and + /// individual importer results in the case where some other component of the + /// importer chain isn't cacheable. + final _perImporterCanonicalizeCache = + <(Importer, Uri, {bool forImport}), CanonicalizeResult?>{}; + + /// A map from the keys in [_perImporterCanonicalizeCache] that are generated + /// for relative URL loads agains the base importer to the original relative + /// URLs what were loaded. /// - /// 1. The URL passed to [canonicalize] (the same as in [_canonicalizeCache]). - /// 2. Whether the canonicalization is for an `@import` rule. - /// 3. The `baseImporter` passed to [canonicalize]. - /// 4. The `baseUrl` passed to [canonicalize]. - /// - /// The map's values are the same as the return value of [canonicalize]. - final _relativeCanonicalizeCache = <( - Uri, { - bool forImport, - Importer baseImporter, - Uri? baseUrl - }), - CanonicalizeResult?>{}; + /// This is used to invalidate the cache when files are changed. + final _nonCanonicalRelativeUrls = <(Importer, Uri, {bool forImport}), Uri>{}; /// The parsed stylesheets for each canonicalized import URL. final _importCache = {}; @@ -155,18 +153,16 @@ final class ImportCache { } if (baseImporter != null && url.scheme == '') { - var relativeResult = _relativeCanonicalizeCache.putIfAbsent(( - url, - forImport: forImport, - baseImporter: baseImporter, - baseUrl: baseUrl - ), () { - var (result, cacheable) = _canonicalize( - baseImporter, baseUrl?.resolveUri(url) ?? url, baseUrl, forImport); + var resolvedUrl = baseUrl?.resolveUri(url) ?? url; + var key = (baseImporter, resolvedUrl, forImport: forImport); + var relativeResult = _perImporterCanonicalizeCache.putIfAbsent(key, () { + var (result, cacheable) = + _canonicalize(baseImporter, resolvedUrl, baseUrl, forImport); assert( cacheable, "Relative loads should always be cacheable because they never " "provide access to the containing URL."); + if (baseUrl != null) _nonCanonicalRelativeUrls[key] = url; return result; }); if (relativeResult != null) return relativeResult; @@ -182,17 +178,41 @@ final class ImportCache { // `canonicalize()` calls we've attempted are cacheable. Only if they are do // we store the result in the cache. var cacheable = true; - for (var importer in _importers) { + for (var i = 0; i < _importers.length; i++) { + var importer = _importers[i]; + var perImporterKey = (importer, url, forImport: forImport); + switch (_perImporterCanonicalizeCache.getOption(perImporterKey)) { + case (var result?,): + return result; + case (null,): + continue; + } + switch (_canonicalize(importer, url, baseUrl, forImport)) { case (var result?, true) when cacheable: _canonicalizeCache[key] = result; return result; - case (var result?, _): - return result; - - case (_, false): - cacheable = false; + case (var result, true) when !cacheable: + _perImporterCanonicalizeCache[perImporterKey] = result; + if (result != null) return result; + + case (var result, false): + if (cacheable) { + // If this is the first uncacheable result, add all previous results + // to the per-importer cache so we don't have to re-run them for + // future uses of this importer. + for (var j = 0; j < i; j++) { + _perImporterCanonicalizeCache[( + _importers[j], + url, + forImport: forImport + )] = null; + } + cacheable = false; + } + + if (result != null) return result; } } @@ -312,7 +332,7 @@ final class ImportCache { Uri sourceMapUrl(Uri canonicalUrl) => _resultsCache[canonicalUrl]?.sourceMapUrl ?? canonicalUrl; - /// Clears the cached canonical version of the given [url]. + /// Clears the cached canonical version of the given non-canonical [url]. /// /// Has no effect if the canonical version of [url] has not been cached. /// @@ -321,7 +341,8 @@ final class ImportCache { void clearCanonicalize(Uri url) { _canonicalizeCache.remove((url, forImport: false)); _canonicalizeCache.remove((url, forImport: true)); - _relativeCanonicalizeCache.removeWhere((key, _) => key.$1 == url); + _perImporterCanonicalizeCache.removeWhere( + (key, _) => key.$2 == url || _nonCanonicalRelativeUrls[key] == url); } /// Clears the cached parse tree for the stylesheet with the given diff --git a/lib/src/util/map.dart b/lib/src/util/map.dart index 70037fd64..46982a79c 100644 --- a/lib/src/util/map.dart +++ b/lib/src/util/map.dart @@ -2,6 +2,8 @@ // MIT-style license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. +import 'option.dart'; + extension MapExtensions on Map { /// If [this] doesn't contain the given [key], sets that key to [value] and /// returns it. @@ -16,4 +18,8 @@ extension MapExtensions on Map { // TODO(nweiz): Remove this once dart-lang/collection#289 is released. /// Like [Map.entries], but returns each entry as a record. Iterable<(K, V)> get pairs => entries.map((e) => (e.key, e.value)); + + /// Returns an option that contains the value at [key] if one exists and null + /// otherwise. + Option getOption(K key) => containsKey(key) ? (this[key]!,) : null; } diff --git a/lib/src/util/option.dart b/lib/src/util/option.dart new file mode 100644 index 000000000..84d296a80 --- /dev/null +++ b/lib/src/util/option.dart @@ -0,0 +1,12 @@ +// Copyright 2024 Google Inc. Use of this source code is governed by an +// MIT-style license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +/// A type that represents either the presence of a value of type `T` or its +/// absence. +/// +/// When the option is present, this will be a single-element tuple that +/// contains the value. If it's absent, it will be null. This allows callers to +/// distinguish between a present null value and a value that's absent +/// altogether. +typedef Option = (T,)?;