Skip to content

Commit

Permalink
Fix cacheKeyForTree & OneShot incompatibility
Browse files Browse the repository at this point in the history 
The fix in #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.
  • Loading branch information
@ef4
ef4 committed Feb 1, 2022
1 parent eaec307 commit ebe4aed
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 108 deletions.
2 changes: 1 addition & 1 deletion packages/compat/src/build-compat-addon.ts
View file
Expand Up @@ -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;
}
Expand Down
13 changes: 1 addition & 12 deletions packages/compat/src/one-shot.ts
View file
Expand Up @@ -17,21 +17,10 @@ class NerfHeimdallBuilder extends Builder {
buildHeimdallTree() {}
}

let seen = new WeakMap<Node, Node>();

// 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}`,
Expand Down
169 changes: 79 additions & 90 deletions packages/compat/src/v1-addon.ts
View file
Expand Up @@ -14,7 +14,6 @@ import { mergeWithAppend } from './merges';
import {
AddonMeta,
NodeTemplateCompiler,
debug,
PackageCache,
Resolver,
extensionsPattern,
Expand Down Expand Up @@ -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
Expand All @@ -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<string>();
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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand All @@ -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.
//
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}
5 changes: 0 additions & 5 deletions packages/compat/src/v1-app.ts
View file
Expand Up @@ -118,11 +118,6 @@ export default class V1App {
return this.requireFromEmberCLI('./lib/utilities/ember-app-utils');
}

@Memoize()
get addonTreeCache(): Map<string, Node> {
return new Map();
}

@Memoize()
get preprocessRegistry() {
return this.requireFromEmberCLI('ember-cli-preprocess-registry/preprocessors');
Expand Down

0 comments on commit ebe4aed

Please sign in to comment.