Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memoization in Broccoli #396

Merged
merged 14 commits into from
May 16, 2019
12 changes: 7 additions & 5 deletions lib/builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,9 +235,9 @@ module.exports = class Builder extends EventEmitter {
} else {
// nodeType === 'source'
if (nodeInfo.watched) {
this.watchedPaths.push(nodeInfo.sourceDirectory);
this.watchedPaths.push({ root: nodeWrapper, path: nodeInfo.sourceDirectory });
thoov marked this conversation as resolved.
Show resolved Hide resolved
} else {
this.unwatchedPaths.push(nodeInfo.sourceDirectory);
this.unwatchedPaths.push({ root: nodeWrapper, path: nodeInfo.sourceDirectory });
}
}

Expand All @@ -263,12 +263,14 @@ module.exports = class Builder extends EventEmitter {
for (let i = 0; i < this.watchedPaths.length; i++) {
let isDirectory;
try {
isDirectory = fs.statSync(this.watchedPaths[i]).isDirectory();
isDirectory = fs.statSync(this.watchedPaths[i].path).isDirectory();
} catch (err) {
throw new this.constructor.BuilderError('Directory not found: ' + this.watchedPaths[i]);
throw new this.constructor.BuilderError(
`Directory not found: ${this.watchedPaths[i].path}`
);
}
if (!isDirectory) {
throw new this.constructor.BuilderError('Not a directory: ' + this.watchedPaths[i]);
throw new this.constructor.BuilderError(`Not a directory: ${this.watchedPaths[i].path}`);
}
}
}
Expand Down
12 changes: 7 additions & 5 deletions lib/watcher_adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ function defaultFilterFunction(name) {
return /^[^.]/.test(name);
}

function bindFileEvent(adapter, watcher, event) {
function bindFileEvent(adapter, watcher, watchedNode, event) {
watcher.on(event, (filepath, root) => {
logger.debug(event, root + '/' + filepath);
watchedNode.revise();
stefanpenner marked this conversation as resolved.
Show resolved Hide resolved
adapter.emit('change');
});
}
Expand All @@ -27,12 +28,13 @@ module.exports = class WatcherAdapter extends EventEmitter {
if (!Array.isArray(watchedPaths)) {
throw new TypeError(`WatcherAdapter#watch's first argument must be an array of watchedPaths`);
}
let watchers = watchedPaths.map(watchedPath => {
let watchers = watchedPaths.map(({ root: watchedNode, path: watchedPath }) => {
const watcher = new sane(watchedPath, this.options);
this.watchers.push(watcher);
bindFileEvent(this, watcher, 'change');
bindFileEvent(this, watcher, 'add');
bindFileEvent(this, watcher, 'delete');
bindFileEvent(this, watcher, watchedNode, 'change');
bindFileEvent(this, watcher, watchedNode, 'add');
bindFileEvent(this, watcher, watchedNode, 'delete');

return new Promise((resolve, reject) => {
watcher.on('ready', resolve);
watcher.on('error', reject);
Expand Down
9 changes: 9 additions & 0 deletions lib/wrappers/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,15 @@ const undefinedToNull = require('../utils/undefined-to-null');
module.exports = class NodeWrapper {
constructor() {
this.buildState = {};
this._revision = 0;
}

revise() {
this._revision++;
}

get revision() {
return this._revision;
}

toJSON() {
Expand Down
53 changes: 53 additions & 0 deletions lib/wrappers/transform-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const NodeWrapper = require('./node');
const fs = require('fs');
const undefinedToNull = require('../utils/undefined-to-null');
const rimraf = require('rimraf');
const logger = require('heimdalljs-logger')('broccoli:wrappers:transform-node');

module.exports = class TransformNodeWrapper extends NodeWrapper {
setup(features) {
Expand All @@ -13,6 +14,40 @@ module.exports = class TransformNodeWrapper extends NodeWrapper {
cachePath: this.cachePath,
});
this.callbackObject = this.nodeInfo.getCallbackObject();
this.callbackObject.revised = () => {
thoov marked this conversation as resolved.
Show resolved Hide resolved
this.revised();
};

// This weakmap holds references from inputNode --> last known revision #
// If the any inputNode's ref does not match what is stored in here then we
// know a modification has happened so we call the build method
this.inputRevisions = new WeakMap();
}

// Memoization is currently optin via either BROCCOLI_VOLATILE = true or
// by a plugin specifing that they are volatile
memoizationEnabled(volatile) {
thoov marked this conversation as resolved.
Show resolved Hide resolved
return volatile || process.env.BROCCOLI_VOLATILE === 'true';
thoov marked this conversation as resolved.
Show resolved Hide resolved
}

shouldBuild() {
let shouldBuild = false;

// The plugin has no input nodes so it's build method should not
// be called after the first build
if (this.inputNodeWrappers.length === 0 && this.revision > 0) {
return false;
}

this.inputNodeWrappers.forEach(wrapper => {
if (this.inputRevisions.get(wrapper) !== wrapper.revision) {
logger.debug(`${wrapper.toString()} has been revised since last build.`);
stefanpenner marked this conversation as resolved.
Show resolved Hide resolved
shouldBuild = true;
this.inputRevisions.set(wrapper, wrapper.revision);
}
});

return shouldBuild;
}

build() {
Expand All @@ -21,14 +56,32 @@ module.exports = class TransformNodeWrapper extends NodeWrapper {
return new Promise(resolve => {
startTime = process.hrtime();

if (this.memoizationEnabled(this.nodeInfo.volatile)) {
thoov marked this conversation as resolved.
Show resolved Hide resolved
let shouldBuild = this.shouldBuild();

if (!shouldBuild) {
logger.debug(
`${this.toString()}'s inputNodes have not been revised. Skipping building this node.`
);

return resolve(); // Noop the build since inputs did not change
}
}

if (!this.nodeInfo.persistentOutput) {
rimraf.sync(this.outputPath);
fs.mkdirSync(this.outputPath);
}

resolve(this.callbackObject.build());

// If the plugin is volatile then it will be responsible for calling revise.
if (!this.nodeInfo.volatile) {
thoov marked this conversation as resolved.
Show resolved Hide resolved
this.revise();
}
}).then(() => {
const now = process.hrtime();

// Build time in milliseconds
this.buildState.selfTime = 1000 * (now[0] - startTime[0] + (now[1] - startTime[1]) / 1e9);
this.buildState.totalTime = this.buildState.selfTime;
Expand Down
75 changes: 71 additions & 4 deletions test/builder_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ describe('Builder', function() {
const plugins = makePlugins(Plugin);

describe('broccoli-plugin ' + version, function() {
afterEach(() => {
delete process.env['BROCCOLI_VOLATILE'];
});

it('builds a single node, repeatedly', function() {
const node = new plugins.Veggies();
const buildSpy = sinon.spy(node, 'build');
Expand Down Expand Up @@ -146,6 +150,65 @@ describe('Builder', function() {
});
});

it('only builds if revision counter has incremented', function() {
process.env['BROCCOLI_VOLATILE'] = true;

const outputNode = new plugins.Merge([
new broccoliSource.WatchedDir('test/fixtures/basic'),
new broccoliSource.WatchedDir('test/fixtures/public'),
]);

const buildSpy = sinon.spy(outputNode, 'build');

builder = new FixtureBuilder(outputNode);

return builder
.build()
.then(() => {
expect(buildSpy).to.have.been.calledOnce;

// Now we simulate a rebuild (and the revisions have not changed)
return builder.build();
})
.then(() => {
expect(buildSpy).to.have.been.calledOnce;
});
});

it('only nodes with inputs that have different revisions call their builds', function() {
process.env['BROCCOLI_VOLATILE'] = true;

const basicWatchDir = new broccoliSource.WatchedDir('test/fixtures/basic');
const publicWatchDir = new broccoliSource.WatchedDir('test/fixtures/public');

const fooNode = new plugins.Merge([basicWatchDir], { overwrite: true });
const barNode = new plugins.Merge([publicWatchDir], { overwrite: true });
const outputNode = new plugins.Merge([fooNode, barNode], { overwrite: true });
const fooBuildSpy = sinon.spy(fooNode, 'build');
const barBuildSpy = sinon.spy(barNode, 'build');
const buildSpy = sinon.spy(outputNode, 'build');

builder = new FixtureBuilder(outputNode);

return builder
.build()
.then(() => {
expect(fooBuildSpy).to.have.been.calledOnce;
expect(barBuildSpy).to.have.been.calledOnce;
expect(buildSpy).to.have.been.calledOnce;

// Now we simulate a rebuild (and the revisions have not changed)
builder.nodeWrappers.find(wrap => wrap.outputPath === 'test/fixtures/basic').revise();

return builder.build();
})
.then(() => {
expect(fooBuildSpy).to.have.been.calledTwice;
expect(barBuildSpy).to.have.been.calledOnce;
expect(buildSpy).to.have.been.calledTwice;
});
});

it('supplies a cachePath by default', function() {
// inputPath and outputPath are tested implicitly by the other tests,
// but cachePath isn't, so we have this test case
Expand Down Expand Up @@ -255,7 +318,9 @@ describe('Builder', function() {
builder = new FixtureBuilder(new broccoliSource.UnwatchedDir('test/fixtures/basic'));

expect(builder.watchedPaths).to.deep.equal([]);
expect(builder.unwatchedPaths).to.deep.equal(['test/fixtures/basic']);
expect(builder.unwatchedPaths.map(paths => paths.path)).to.deep.equal([
stefanpenner marked this conversation as resolved.
Show resolved Hide resolved
'test/fixtures/basic',
]);

return expect(builder.build()).to.eventually.deep.equal({
'foo.txt': 'OK',
Expand All @@ -265,7 +330,9 @@ describe('Builder', function() {
it('records watched source directories', function() {
builder = new FixtureBuilder(new broccoliSource.WatchedDir('test/fixtures/basic'));

expect(builder.watchedPaths).to.deep.equal(['test/fixtures/basic']);
expect(builder.watchedPaths.map(paths => paths.path)).to.deep.equal([
'test/fixtures/basic',
]);
expect(builder.unwatchedPaths).to.deep.equal([]);

return expect(builder.build()).to.eventually.deep.equal({
Expand All @@ -278,7 +345,7 @@ describe('Builder', function() {
it('records string (watched) source directories', function() {
builder = new FixtureBuilder('test/fixtures/basic');

expect(builder.watchedPaths).to.deep.equal(['test/fixtures/basic']);
expect(builder.watchedPaths.map(paths => paths.path)).to.deep.equal(['test/fixtures/basic']);
expect(builder.unwatchedPaths).to.deep.equal([]);

return expect(builder.build()).to.eventually.deep.equal({
Expand All @@ -291,7 +358,7 @@ describe('Builder', function() {

builder = new FixtureBuilder(new plugins.Merge([src, src]));

expect(builder.watchedPaths).to.deep.equal(['test/fixtures/basic']);
expect(builder.watchedPaths.map(paths => paths.path)).to.deep.equal(['test/fixtures/basic']);
});

it("fails construction when a watched source directory doesn't exist", function() {
Expand Down
23 changes: 18 additions & 5 deletions test/watch_adapter_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,31 +26,40 @@ describe('WatcherAdapter', function() {
},
};

const watchRoot = {
revise() {},
};

it('works', function() {
const trigger = sinon.spy(adapter, 'emit');
const on = sinon.spy(watcher, 'on');
const revise = sinon.spy(watchRoot, 'revise');

expect(on).to.have.not.been.called;
expect(trigger).to.have.not.been.called;
expect(revise).to.have.not.been.called;

bindFileEvent(adapter, watcher, 'change');
bindFileEvent(adapter, watcher, watchRoot, 'change');

expect(on).to.have.been.calledOnce;
expect(trigger).to.have.been.calledOnce;
expect(revise).to.have.been.calledOnce;
expect(on).to.have.been.calledWith('change');
expect(trigger).to.have.been.calledWith('change');

bindFileEvent(adapter, watcher, 'add');
bindFileEvent(adapter, watcher, watchRoot, 'add');

expect(on).to.have.been.calledTwice;
expect(trigger).to.have.been.calledTwice;
expect(revise).to.have.been.calledTwice;
expect(on).to.have.been.calledWith('add');
expect(trigger).to.have.been.calledWith('change');

bindFileEvent(adapter, watcher, 'remove');
bindFileEvent(adapter, watcher, watchRoot, 'remove');

expect(on).to.have.been.calledThrice;
expect(trigger).to.have.been.calledThrice;
expect(revise).to.have.been.calledThrice;
expect(on).to.have.been.calledWith('remove');
expect(trigger).to.have.been.calledWith('change');
});
Expand Down Expand Up @@ -93,6 +102,10 @@ describe('WatcherAdapter', function() {
const FIXTURE_PROJECT = __dirname + '/fixtures/project';
let adapter;

const watchRoot = {
revise() {},
};

afterEach(function() {
adapter.quit();
});
Expand Down Expand Up @@ -127,7 +140,7 @@ describe('WatcherAdapter', function() {
expect(trigger).to.have.callCount(0);

expect(adapter.watchers.length).to.eql(0);
let watching = adapter.watch([FIXTURE_BASIC]);
let watching = adapter.watch([{ path: FIXTURE_BASIC, root: watchRoot }]);

expect(adapter.watchers.length).to.eql(1);

Expand All @@ -145,7 +158,7 @@ describe('WatcherAdapter', function() {
trigger.resetHistory();

// this time also watch the FIXTURE_PROJECT
let watching = adapter.watch([FIXTURE_PROJECT]);
let watching = adapter.watch([{ path: FIXTURE_PROJECT, root: watchRoot }]);
expect(adapter.watchers.length).to.eql(2);

return watching.then(val => {
Expand Down
Loading