From ebe4aed33220e070b3b69cf34b87917da91f01a3 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Tue, 1 Feb 2022 18:23:34 -0500 Subject: [PATCH] Fix cacheKeyForTree & OneShot incompatibility The fix in https://github.com/embroider-build/embroider/pull/1064 turns out to be insufficient. The problem is that `cacheKeyForTree` can provide a cached tree at any point in the broccoli graph. We can't tell just by looking at the top level node for an addon whether it contains, deep down inside it, some trees that have already been run by other builders via OneShot. I don't like `cacheKeyForTree`. It's trying to solve a real problem, but at the wrong layer. Instead of sharing work between many addon instances, it should have been preventing there from *being* many addon instances in the first place. I'm moving Embroider to a solution more like that. We already have the `reduceInstances` hook, which is a more blunt and powerful way to through away whole Addon instances. We can give it a default implementation that will allow two addons with *entirely* identical cacheKeyForTree to deduplicate at the *instance* level, not the tree level. --- packages/compat/src/build-compat-addon.ts | 2 +- packages/compat/src/one-shot.ts | 13 +- packages/compat/src/v1-addon.ts | 169 ++++++++++------------ packages/compat/src/v1-app.ts | 5 - 4 files changed, 81 insertions(+), 108 deletions(-) diff --git a/packages/compat/src/build-compat-addon.ts b/packages/compat/src/build-compat-addon.ts index df9f2caa0..451bc9b2d 100644 --- a/packages/compat/src/build-compat-addon.ts +++ b/packages/compat/src/build-compat-addon.ts @@ -11,7 +11,7 @@ import EmptyPackageTree from './empty-package-tree'; export default function cachedBuildCompatAddon(originalPackage: Package, v1Cache: V1InstanceCache): Node { let tree = buildCompatAddon(originalPackage, v1Cache); if (!originalPackage.mayRebuild) { - tree = OneShot.create(tree, originalPackage.name); + tree = new OneShot(tree, originalPackage.name); } return tree; } diff --git a/packages/compat/src/one-shot.ts b/packages/compat/src/one-shot.ts index 4a993bc19..11564c682 100644 --- a/packages/compat/src/one-shot.ts +++ b/packages/compat/src/one-shot.ts @@ -17,21 +17,10 @@ class NerfHeimdallBuilder extends Builder { buildHeimdallTree() {} } -let seen = new WeakMap(); - // Wraps a broccoli tree such that it (and everything it depends on) will only // build a single time. export default class OneShot extends Plugin { - static create(originalTree: Node, privateAddonName: string) { - let output = seen.get(originalTree); - if (!output) { - output = new this(originalTree, privateAddonName); - seen.set(originalTree, output); - } - return output; - } - - private constructor(private inner: Node | null, private addonName: string) { + constructor(private inner: Node | null, private addonName: string) { // from broccoli's perspective, we don't depend on any input trees! super([], { annotation: `@embroider/compat: ${addonName}`, diff --git a/packages/compat/src/v1-addon.ts b/packages/compat/src/v1-addon.ts index 6e48d0011..410348c42 100644 --- a/packages/compat/src/v1-addon.ts +++ b/packages/compat/src/v1-addon.ts @@ -14,7 +14,6 @@ import { mergeWithAppend } from './merges'; import { AddonMeta, NodeTemplateCompiler, - debug, PackageCache, Resolver, extensionsPattern, @@ -135,7 +134,7 @@ export default class V1Addon { } // Optional extensible hook for pruning down the list of redundant addon - // instances produces by the classic ember-cli architecture. ember-cli + // instances produced by the classic ember-cli architecture. ember-cli // instantiates each addon *per consumer*, not per package. So a given package // will have many addon instances, and Embroider dutifully produces a V1Addon // instance for each one, and then needs to mimic the classic smooshing @@ -150,8 +149,24 @@ export default class V1Addon { // the highest precedence, meaning its files would win under classic // smooshing. reduceInstances(instances: V1Addon[]): V1Addon[] { - // the default beahvior is that all copies matter - return instances; + // Our default implementation will deduplicate instances that have the same + // cacheKeyForTree for *all* the known tree names. + let seenKeys = new Set(); + let keptInstances: V1Addon[] = []; + for (let instance of instances) { + let key = cacheKeyForAddon(instance.addonInstance); + if (!key) { + // entirely uncacheable + keptInstances.push(instance); + } else if (!seenKeys.has(key)) { + // cacheable and new + keptInstances.push(instance); + seenKeys.add(key); + } else { + // dropped as redundant + } + } + return keptInstances; } // this is only defined when there are custom AST transforms that need it @@ -454,14 +469,12 @@ export default class V1Addon { } protected stockTree(treeName: AddonTreePath): Node { - return this.throughTreeCache(treeName, 'stock', () => { - // adjust from the legacy "root" to our real root, because our rootTree - // uses our real root but the stock trees are defined in terms of the - // legacy root - let srcDir = relative(this.root, join(this.addonInstance.root, this.addonInstance.treePaths[treeName])); - let opts = Object.assign({ srcDir }, this.stockTreeFunnelOptions(treeName)); - return buildFunnel(this.rootTree, opts); - })!; + // adjust from the legacy "root" to our real root, because our rootTree + // uses our real root but the stock trees are defined in terms of the + // legacy root + let srcDir = relative(this.root, join(this.addonInstance.root, this.addonInstance.treePaths[treeName])); + let opts = Object.assign({ srcDir }, this.stockTreeFunnelOptions(treeName)); + return buildFunnel(this.rootTree, opts); } @Memoize() @@ -547,25 +560,7 @@ export default class V1Addon { } get v2Tree(): Node { - return this.throughTreeCache( - // these are all the kinds of trees that ember-cli's tree cache - // understands. We need them all here because if *any* of these are - // uncacheable, we want our whole v2 tree to be treated as uncacheable. - [ - 'app', - 'addon', - 'addon-styles', - 'addon-templates', - 'addon-test-support', - 'public', - 'styles', - 'templates', - 'test-support', - 'vendor', - ], - 'v2Tree', - () => mergeTrees(this.v2Trees, { overwrite: true }) - ); + return mergeTrees(this.v2Trees, { overwrite: true }); } // this is split out so that compatibility shims can override it to add more @@ -589,44 +584,6 @@ export default class V1Addon { return trees; } - protected throughTreeCache(nameOrNames: string | string[], category: string, fn: () => Node): Node; - protected throughTreeCache( - nameOrNames: string | string[], - category: string, - fn: () => Node | undefined - ): Node | undefined { - let cacheKey: string | undefined; - if (typeof this.addonInstance.cacheKeyForTree === 'function') { - let names = Array.isArray(nameOrNames) ? nameOrNames : [nameOrNames]; - cacheKey = names.reduce((accum: string | undefined, name) => { - if (accum == null) { - // a previous name was uncacheable, so we're entirely uncacheable - return undefined; - } - let key = this.addonInstance.cacheKeyForTree?.(name); - if (key) { - return accum + key; - } else { - return undefined; - } - }, ''); - if (cacheKey) { - cacheKey = cacheKey + category; - let cachedTree = this.app.addonTreeCache.get(cacheKey); - if (cachedTree) { - debug('cache hit %s %s %s', this.name, nameOrNames, category); - return cachedTree; - } - } - } - debug('cache miss %s %s %s', this.name, nameOrNames, category); - let tree = fn(); - if (tree && cacheKey) { - this.app.addonTreeCache.set(cacheKey, tree); - } - return tree; - } - // In general, we can't reliably run addons' custom `treeFor()` methods, // because they recurse in a way that we absolutely don't want. // @@ -667,28 +624,25 @@ export default class V1Addon { name: string, { neuterPreprocessors } = { neuterPreprocessors: false } ): Node | undefined { - // @ts-expect-error have no idea why throughTreeCache overload is not working here.. - return this.throughTreeCache(name, 'original', () => { - // get the real addon as we're going to patch and restore `preprocessJs` - const realAddon = getRealAddon(this.addonInstance); - let original; - try { - if (neuterPreprocessors) { - original = realAddon.preprocessJs; - realAddon.preprocessJs = function (tree: Node) { - return tree; - }; - } - if (this.suppressesTree(name)) { - return undefined; - } - return this.addonInstance._treeFor(name); - } finally { - if (neuterPreprocessors) { - realAddon.preprocessJs = original; - } + // get the real addon as we're going to patch and restore `preprocessJs` + const realAddon = getRealAddon(this.addonInstance); + let original; + try { + if (neuterPreprocessors) { + original = realAddon.preprocessJs; + realAddon.preprocessJs = function (tree: Node) { + return tree; + }; } - }); + if (this.suppressesTree(name)) { + return undefined; + } + return this.addonInstance._treeFor(name); + } finally { + if (neuterPreprocessors) { + realAddon.preprocessJs = original; + } + } } protected treeForAddon(built: IntermediateBuild): Node | undefined { @@ -1132,3 +1086,38 @@ const stubbedSuper = () => { stubbedSuper.treeFor = () => { return markedEmptyTree; }; + +function cacheKeyForAddon(instance: AddonInstance): string | undefined { + // these are all the kinds of trees that ember-cli's tree cache understands. + // We deduplicate at the level of whole instances, not individual trees, so + // our cache key in the combination of all of these. + let names = [ + 'app', + 'addon', + 'addon-styles', + 'addon-templates', + 'addon-test-support', + 'public', + 'styles', + 'templates', + 'test-support', + 'vendor', + ]; + + let cacheKey: string | undefined; + if (typeof instance.cacheKeyForTree === 'function') { + cacheKey = names.reduce((accum: string | undefined, name) => { + if (accum == null) { + // a previous name was uncacheable, so we're entirely uncacheable + return undefined; + } + let key = instance.cacheKeyForTree?.(name); + if (key) { + return accum + key; + } else { + return undefined; + } + }, ''); + } + return cacheKey; +} diff --git a/packages/compat/src/v1-app.ts b/packages/compat/src/v1-app.ts index 5fe07ed87..874cbdbd9 100644 --- a/packages/compat/src/v1-app.ts +++ b/packages/compat/src/v1-app.ts @@ -118,11 +118,6 @@ export default class V1App { return this.requireFromEmberCLI('./lib/utilities/ember-app-utils'); } - @Memoize() - get addonTreeCache(): Map { - return new Map(); - } - @Memoize() get preprocessRegistry() { return this.requireFromEmberCLI('ember-cli-preprocess-registry/preprocessors');