From fabb20d2cde1e85d71c6730fdede5a3bbfc1e2ac Mon Sep 17 00:00:00 2001 From: David Greenspan Date: Thu, 4 Feb 2016 17:10:44 -0800 Subject: [PATCH] Rebuild in place for better performance writeSiteArchive already supported rebuilding in place (rather than creating a new build directory and moving it into place; see comments at top of builder.js), but it was not used and didn't work: * run-app.js only passed previousBuilders to the bundler in the case of a client-only refresh, in which case it also passed hasCachedBundle, bypassing writeSiteArchive altogether. * writeSiteArchive's use of previousBuilders seemingly didn't work, because the site archive itself was never written in place, so trying to write the targets "in place" into a brand-new build directory didn't make sense (and threw an error). With this change, previousBuilders are kept across all rebuilds (not just client-only refreshes), which enables efficient, in-place building (except on Windows, where in-place building has never been supported), and writeSiteArchive is fixed to write the site archive in place as well. --- tools/isobuild/builder.js | 72 ++++++++++++++++++++++++++++----------- tools/isobuild/bundler.js | 5 ++- tools/runners/run-app.js | 11 +++--- 3 files changed, 62 insertions(+), 26 deletions(-) diff --git a/tools/isobuild/builder.js b/tools/isobuild/builder.js index 85ccc28c94..8de34c6bb3 100644 --- a/tools/isobuild/builder.js +++ b/tools/isobuild/builder.js @@ -3,29 +3,45 @@ import files from '../fs/files.js'; import NpmDiscards from './npm-discards.js'; import {Profile} from '../tool-env/profile.js'; -// Builder has two modes of working: -// - write files to a temp directory and later atomically move it to destination -// - write files in-place replacing the older files -// The later doesn't work on Windows but works well on Mac OS X and Linux, since -// the file system allows writing new files to the path of a file opened by a -// process. The process only retains the inode, not the path. +// Builder is in charge of writing "bundles" to disk, which are +// directory trees such as site archives, programs, and packages. In +// addition to writing data to files, it can copy or link in existing +// files and directories (keeping track of them in a WatchSet in order +// to trigger rebuilds appropriately). +// +// By default, Builder constructs the entire output directory from +// scratch under a temporary name, and then moves it into place. +// For efficient rebuilds, Builder can be given a `previousBuilder`, +// in which case it will write files into the existing output directory +// instead. +// +// On Windows (or when METEOR_DISABLE_BUILDER_IN_PLACE is set), Builder +// always creates a new output directory under a temporary name rather than +// using the old directory. The reason is that we don't want rebuilding to +// interfere with the running app, and we rely on the fact that on OS X and +// Linux, if the process has opened a file for reading, it retains the file +// by its inode, not path, so it is safe to write a new file to the same path +// (or delete the file). +// +// Separate from that, Builder has a strategy of writing files under a temporary +// name and then renaming them. This is to achieve an "atomic" write, meaning +// the server doesn't see a partially-written file that appears truncated. +// +// On Windows we copy files instead of symlinking them (see comments inline). + + +// Whether to support writing files into the same directory as a previous +// Builder on rebuild (rather than creating a new build directory and +// moving it into place). const ENABLE_IN_PLACE_BUILDER_REPLACEMENT = (process.platform !== 'win32') && ! process.env.METEOR_DISABLE_BUILDER_IN_PLACE; -// Builder encapsulates much of the file-handling logic need to create -// "bundles" (directory trees such as site archives, programs, or -// packages). It can create a temporary directory in which to build -// the bundle, moving the bundle atomically into place when and if the -// build successfully completes; sanitize and generate unique -// filenames; and track dependencies (files that should be watched for -// changes when developing interactively). -// // Options: // - outputPath: Required. Path to the directory that will hold the -// bundle when building is complete. It should not exist. Its -// parents will be created if necessary. +// bundle when building is complete. It should not exist (unless +// previousBuilder is passed). Its parents will be created if necessary. // - previousBuilder: Optional. An in-memory instance of Builder left // from the previous iteration. It is assumed that the previous builder // has completed its job successfully and its files are stored on the @@ -239,7 +255,7 @@ Previous builder: ${previousBuilder.outputPath}, this builder: ${outputPath}` const absPath = files.pathJoin(this.buildPath, relPath); if (symlink) { - files.symlink(symlink, absPath); + symlinkWithOverwrite(symlink, absPath); } else { hash = hash || sha1(getData()); @@ -425,7 +441,7 @@ Previous builder: ${previousBuilder.outputPath}, this builder: ${outputPath}` if (canSymlink) { this._ensureDirectory(files.pathDirname(to)); - files.symlink(files.pathResolve(from), absPathTo); + symlinkWithOverwrite(files.pathResolve(from), absPathTo); return; } } @@ -474,8 +490,8 @@ Previous builder: ${previousBuilder.outputPath}, this builder: ${outputPath}` if (isDirectory) { walk(thisAbsFrom, thisRelTo); } else if (fileStatus.isSymbolicLink()) { - files.symlink(files.readlink(thisAbsFrom), - files.pathResolve(this.buildPath, thisRelTo)); + symlinkWithOverwrite(files.readlink(thisAbsFrom), + files.pathResolve(this.buildPath, thisRelTo)); // A symlink counts as a file, as far as "can you put something under // it" goes. this.usedAsFile[thisRelTo] = true; @@ -622,6 +638,22 @@ function atomicallyRewriteFile(path, data, options) { } } +// create a symlink, overwriting the target link, file, or directory +// if it exists +function symlinkWithOverwrite(source, target) { + try { + files.symlink(source, target); + } catch (e) { + if (e.code === 'EEXIST') { + // overwrite existing link, file, or directory + files.rm_recursive(target); + files.symlink(source, target); + } else { + throw e; + } + } +} + // Wrap slow methods into Profiler calls const slowBuilderMethods = [ '_ensureDirectory', 'write', 'enter', 'copyDirectory', 'enter', 'complete' diff --git a/tools/isobuild/bundler.js b/tools/isobuild/bundler.js index 30eeced6ca..da04af4724 100644 --- a/tools/isobuild/bundler.js +++ b/tools/isobuild/bundler.js @@ -1959,7 +1959,10 @@ var writeSiteArchive = Profile("bundler writeSiteArchive", function ( }) { const builders = {}; - const builder = new Builder({outputPath}); + const previousStarBuilder = previousBuilders && previousBuilders.star; + const builder = new Builder({outputPath, + previousBuilder: previousStarBuilder}); + builders.star = builder; try { var json = { diff --git a/tools/runners/run-app.js b/tools/runners/run-app.js index 7f71e994e6..e46b1cad26 100644 --- a/tools/runners/run-app.js +++ b/tools/runners/run-app.js @@ -384,6 +384,10 @@ var AppRunner = function (options) { // is communicating to the app process over ipc. If an error in communication // occurs, we can distinguish it in a callback handling the 'error' event. self._refreshing = false; + + // Builders saved across rebuilds, so that targets can be re-written in + // place instead of created again from scratch. + self.builders = {}; }; _.extend(AppRunner.prototype, { @@ -491,9 +495,6 @@ _.extend(AppRunner.prototype, { // a single invocation of _runOnce(). var cachedServerWatchSet; - // Builders saved from previous iterations - var builders = {}; - var bundleApp = function () { if (! firstRun) { // If the build fails in a way that could be fixed by a refresh, allow @@ -588,11 +589,11 @@ _.extend(AppRunner.prototype, { includeNodeModules: includeNodeModules, buildOptions: self.buildOptions, hasCachedBundle: !! cachedServerWatchSet, - previousBuilders: builders + previousBuilders: self.builders }); // save new builders with their caches - ({builders} = bundleResult); + self.builders = bundleResult.builders; return bundleResult; });