From 5b040a52c4479236991afdf7037653342539037e Mon Sep 17 00:00:00 2001 From: Jing Chen Date: Fri, 12 Aug 2016 16:55:59 -0700 Subject: [PATCH] Reverted commit D3703896 Summary: This is a very hacky solution to make reloads from packager faster for simple file changes. Simple means that no dependencies have changed, otherwise packager will abort the attempt to update and fall back to the usual rebuilding method. In principle, this change avoids re-walking and analyzing the whole dependency tree, and just updates modules in existing bundles. Reviewed By: bestander Differential Revision: D3703896 fbshipit-source-id: abc2a41144536baf969d346522a17044c1c9558b --- packager/react-packager/src/Bundler/Bundle.js | 2 +- .../react-packager/src/Bundler/BundleBase.js | 10 +- .../src/Bundler/__tests__/Bundler-test.js | 11 +- packager/react-packager/src/Bundler/index.js | 17 +- packager/react-packager/src/Server/index.js | 171 ++---------------- .../DependencyGraph/ResolutionResponse.js | 8 +- 6 files changed, 27 insertions(+), 192 deletions(-) diff --git a/packager/react-packager/src/Bundler/Bundle.js b/packager/react-packager/src/Bundler/Bundle.js index 833bfe98c..ccaaffa88 100644 --- a/packager/react-packager/src/Bundler/Bundle.js +++ b/packager/react-packager/src/Bundler/Bundle.js @@ -59,7 +59,7 @@ class Bundle extends BundleBase { this._addRequireCall(super.getMainModuleId()); } - super.finalize(options); + super.finalize(); } _addRequireCall(moduleId) { diff --git a/packager/react-packager/src/Bundler/BundleBase.js b/packager/react-packager/src/Bundler/BundleBase.js index f086dc7db..838ffa1d6 100644 --- a/packager/react-packager/src/Bundler/BundleBase.js +++ b/packager/react-packager/src/Bundler/BundleBase.js @@ -59,10 +59,8 @@ class BundleBase { } finalize(options) { - if (!options.allowUpdates) { - Object.freeze(this._modules); - Object.freeze(this._assets); - } + Object.freeze(this._modules); + Object.freeze(this._assets); this._finalized = true; } @@ -78,10 +76,6 @@ class BundleBase { return this._source; } - invalidateSource() { - this._source = null; - } - assertFinalized(message) { if (!this._finalized) { throw new Error(message || 'Bundle needs to be finalized before getting any source'); diff --git a/packager/react-packager/src/Bundler/__tests__/Bundler-test.js b/packager/react-packager/src/Bundler/__tests__/Bundler-test.js index dbc00f759..bbf71a30f 100644 --- a/packager/react-packager/src/Bundler/__tests__/Bundler-test.js +++ b/packager/react-packager/src/Bundler/__tests__/Bundler-test.js @@ -129,7 +129,6 @@ describe('Bundler', function() { dependencies: modules, transformOptions, getModuleId: () => 123, - getResolvedDependencyPairs: () => [], }) ); @@ -142,7 +141,7 @@ describe('Bundler', function() { }); }); - it('create a bundle', function() { + pit('create a bundle', function() { assetServer.getAssetData.mockImpl(() => { return { scales: [1,2,3], @@ -171,11 +170,9 @@ describe('Bundler', function() { expect(ithAddedModule(3)).toEqual('/root/img/new_image.png'); expect(ithAddedModule(4)).toEqual('/root/file.json'); - expect(bundle.finalize.mock.calls[0]).toEqual([{ - runMainModule: true, - runBeforeMainModule: [], - allowUpdates: false, - }]); + expect(bundle.finalize.mock.calls[0]).toEqual([ + {runMainModule: true, runBeforeMainModule: []} + ]); expect(bundle.addAsset.mock.calls[0]).toEqual([{ __packager_asset: true, diff --git a/packager/react-packager/src/Bundler/index.js b/packager/react-packager/src/Bundler/index.js index 27c424301..2056f5138 100644 --- a/packager/react-packager/src/Bundler/index.js +++ b/packager/react-packager/src/Bundler/index.js @@ -89,10 +89,6 @@ const validateOpts = declareOpts({ type: 'boolean', default: false, }, - allowBundleUpdates: { - type: 'boolean', - default: false, - }, }); const assetPropertyBlacklist = new Set([ @@ -284,7 +280,6 @@ class Bundler { bundle.finalize({ runMainModule, runBeforeMainModule: runBeforeMainModuleIds, - allowUpdates: this._opts.allowBundleUpdates, }); return bundle; }); @@ -407,7 +402,6 @@ class Bundler { entryFilePath, transformOptions: response.transformOptions, getModuleId: response.getModuleId, - dependencyPairs: response.getResolvedDependencyPairs(module), }).then(transformed => { modulesByName[transformed.name] = module; onModuleTransformed({ @@ -539,14 +533,7 @@ class Bundler { ); } - _toModuleTransport({ - module, - bundle, - entryFilePath, - transformOptions, - getModuleId, - dependencyPairs, - }) { + _toModuleTransport({module, bundle, entryFilePath, transformOptions, getModuleId}) { let moduleTransport; const moduleId = getModuleId(module); @@ -579,7 +566,7 @@ class Bundler { id: moduleId, code, map, - meta: {dependencies, dependencyOffsets, preloaded, dependencyPairs}, + meta: {dependencies, dependencyOffsets, preloaded}, sourceCode: source, sourcePath: module.path }); diff --git a/packager/react-packager/src/Server/index.js b/packager/react-packager/src/Server/index.js index b957c846a..87db0787f 100644 --- a/packager/react-packager/src/Server/index.js +++ b/packager/react-packager/src/Server/index.js @@ -22,18 +22,11 @@ const mime = require('mime-types'); const path = require('path'); const url = require('url'); -const debug = require('debug')('ReactNativePackager:Server'); - -function debounceAndBatch(fn, delay) { - let timeout, args = []; - return (value) => { - args.push(value); +function debounce(fn, delay) { + var timeout; + return () => { clearTimeout(timeout); - timeout = setTimeout(() => { - const a = args; - args = []; - fn(a); - }, delay); + timeout = setTimeout(fn, delay); }; } @@ -146,10 +139,7 @@ const bundleOpts = declareOpts({ isolateModuleIDs: { type: 'boolean', default: false - }, - resolutionResponse: { - type: 'object', - }, + } }); const dependencyOpts = declareOpts({ @@ -173,14 +163,8 @@ const dependencyOpts = declareOpts({ type: 'boolean', default: false, }, - minify: { - type: 'boolean', - default: undefined, - }, }); -const bundleDeps = new WeakMap(); - class Server { constructor(options) { const opts = validateOpts(options); @@ -225,27 +209,12 @@ class Server { const bundlerOpts = Object.create(opts); bundlerOpts.fileWatcher = this._fileWatcher; bundlerOpts.assetServer = this._assetServer; - bundlerOpts.allowBundleUpdates = !options.nonPersistent; this._bundler = new Bundler(bundlerOpts); this._fileWatcher.on('all', this._onFileChange.bind(this)); - this._debouncedFileChangeHandler = debounceAndBatch(filePaths => { - // only clear bundles for non-JS changes - if (filePaths.every(RegExp.prototype.test, /\.js(?:on)?$/i)) { - for (const key in this._bundles) { - this._bundles[key].then(bundle => { - const deps = bundleDeps.get(bundle); - filePaths.forEach(filePath => { - if (deps.files.has(filePath)) { - deps.outdated.add(filePath); - } - }); - }); - } - } else { - this._clearBundles(); - } + this._debouncedFileChangeHandler = debounce(filePath => { + this._clearBundles(); this._informChangeWatchers(); }, 50); } @@ -274,25 +243,7 @@ class Server { } const opts = bundleOpts(options); - const building = this._bundler.bundle(opts); - building.then(bundle => { - const modules = bundle.getModules().filter(m => !m.virtual); - bundleDeps.set(bundle, { - files: new Map( - modules - .map(({sourcePath, meta = {dependencies: []}}) => - [sourcePath, meta.dependencies]) - ), - idToIndex: new Map(modules.map(({id}, i) => [id, i])), - dependencyPairs: new Map( - modules - .filter(({meta}) => meta && meta.dependencyPairs) - .map(m => [m.sourcePath, m.meta.dependencyPairs]) - ), - outdated: new Set(), - }); - }); - return building; + return this._bundler.bundle(opts); }); } @@ -477,91 +428,6 @@ class Server { ).done(() => Activity.endEvent(assetEvent)); } - _useCachedOrUpdateOrCreateBundle(options) { - const optionsJson = JSON.stringify(options); - const bundleFromScratch = () => { - const building = this.buildBundle(options); - this._bundles[optionsJson] = building; - return building; - }; - - if (optionsJson in this._bundles) { - return this._bundles[optionsJson].then(bundle => { - const deps = bundleDeps.get(bundle); - const {dependencyPairs, files, idToIndex, outdated} = deps; - if (outdated.size) { - debug('Attempt to update existing bundle'); - const changedModules = - Array.from(outdated, this.getModuleForPath, this); - deps.outdated = new Set(); - - const opts = bundleOpts(options); - const {platform, dev, minify, hot} = opts; - - // Need to create a resolution response to pass to the bundler - // to process requires after transform. By providing a - // specific response we can compute a non recursive one which - // is the least we need and improve performance. - const bundlePromise = this._bundles[optionsJson] = - this.getDependencies({ - platform, dev, hot, minify, - entryFile: options.entryFile, - recursive: false, - }).then(response => { - debug('Update bundle: rebuild shallow bundle'); - - changedModules.forEach(m => { - response.setResolvedDependencyPairs( - m, - dependencyPairs.get(m.path), - {ignoreFinalized: true}, - ); - }); - - return this.buildBundle({ - ...options, - resolutionResponse: response.copy({ - dependencies: changedModules, - }) - }).then(updateBundle => { - const oldModules = bundle.getModules(); - const newModules = updateBundle.getModules(); - for (let i = 0, n = newModules.length; i < n; i++) { - const moduleTransport = newModules[i]; - const {meta, sourcePath} = moduleTransport; - if (outdated.has(sourcePath)) { - if (!contentsEqual(meta.dependencies, new Set(files.get(sourcePath)))) { - // bail out if any dependencies changed - return Promise.reject(Error( - `Dependencies of ${sourcePath} changed from [${ - files.get(sourcePath).join(', ') - }] to [${meta.dependencies.join(', ')}]` - )); - } - - oldModules[idToIndex.get(moduleTransport.id)] = moduleTransport; - } - } - - bundle.invalidateSource(); - debug('Successfully updated existing bundle'); - return bundle; - }); - }).catch(e => { - debug('Failed to update existing bundle, rebuilding...', e.stack || e.message); - return bundleFromScratch(); - }); - return bundlePromise; - } else { - debug('Using cached bundle'); - return bundle; - } - }); - } - - return bundleFromScratch(); - } - processRequest(req, res, next) { const urlObj = url.parse(req.url, true); const pathname = urlObj.pathname; @@ -592,29 +458,26 @@ class Server { const startReqEventId = Activity.startEvent('request:' + req.url); const options = this._getOptionsFromUrl(req.url); - debug('Getting bundle for request'); - const building = this._useCachedOrUpdateOrCreateBundle(options); + const optionsJson = JSON.stringify(options); + const building = this._bundles[optionsJson] || this.buildBundle(options); + + this._bundles[optionsJson] = building; building.then( p => { if (requestType === 'bundle') { - debug('Generating source code'); const bundleSource = p.getSource({ inlineSourceMap: options.inlineSourceMap, minify: options.minify, dev: options.dev, }); - debug('Writing response headers'); res.setHeader('Content-Type', 'application/javascript'); res.setHeader('ETag', p.getEtag()); if (req.headers['if-none-match'] === res.getHeader('ETag')){ - debug('Responding with 304'); res.statusCode = 304; res.end(); } else { - debug('Writing request body'); res.end(bundleSource); } - debug('Finished response'); Activity.endEvent(startReqEventId); } else if (requestType === 'map') { let sourceMap = p.getSourceMap({ @@ -636,7 +499,7 @@ class Server { Activity.endEvent(startReqEventId); } }, - error => this._handleError(res, JSON.stringify(options), error) + this._handleError.bind(this, res, optionsJson) ).done(); } @@ -701,7 +564,9 @@ class Server { _sourceMapForURL(reqUrl) { const options = this._getOptionsFromUrl(reqUrl); - const building = this._useCachedOrUpdateOrCreateBundle(options); + const optionsJson = JSON.stringify(options); + const building = this._bundles[optionsJson] || this.buildBundle(options); + this._bundles[optionsJson] = building; return building.then(p => { const sourceMap = p.getSourceMap({ minify: options.minify, @@ -794,8 +659,4 @@ class Server { } } -function contentsEqual(array, set) { - return array.length === set.size && array.every(set.has, set); -} - module.exports = Server; diff --git a/packager/react-packager/src/node-haste/DependencyGraph/ResolutionResponse.js b/packager/react-packager/src/node-haste/DependencyGraph/ResolutionResponse.js index 28740746f..fdee65c73 100644 --- a/packager/react-packager/src/node-haste/DependencyGraph/ResolutionResponse.js +++ b/packager/react-packager/src/node-haste/DependencyGraph/ResolutionResponse.js @@ -8,8 +8,6 @@ */ 'use strict'; -const NO_OPTIONS = {}; - class ResolutionResponse { constructor({transformOptions}) { this.transformOptions = transformOptions; @@ -78,10 +76,8 @@ class ResolutionResponse { this.numPrependedDependencies += 1; } - setResolvedDependencyPairs(module, pairs, options = NO_OPTIONS) { - if (!options.ignoreFinalized) { - this._assertNotFinalized(); - } + setResolvedDependencyPairs(module, pairs) { + this._assertNotFinalized(); const hash = module.hash(); if (this._mappings[hash] == null) { this._mappings[hash] = pairs;