From bdacb9595b92ce2f50c596ea2b3aff85ce093c3b Mon Sep 17 00:00:00 2001 From: Jean Lauliac Date: Mon, 13 Mar 2017 05:17:54 -0700 Subject: [PATCH] packager: Bundler: refactor the maxWorkerCount Summary: The function giving the worker count was duplicated, let's just pass it down from a central place, the Bundler. Also, I simplified the function to use a simple formula rather than arbitrary ranges (it's still arbitrary, just a tad bit less :D ). Reviewed By: davidaurelio Differential Revision: D4689366 fbshipit-source-id: fe5b349396f1a07858f4f80ccaa63c375122fac8 --- .../src/Bundler/__tests__/Bundler-test.js | 15 +++++++++ packages/metro-bundler/src/Bundler/index.js | 27 +++++++++++++++- .../metro-bundler/src/JSTransformer/index.js | 23 ++----------- packages/metro-bundler/src/Resolver/index.js | 1 + .../__tests__/DependencyGraph-test.js | 2 +- .../metro-bundler/src/node-haste/index.js | 32 +++---------------- 6 files changed, 50 insertions(+), 50 deletions(-) diff --git a/packages/metro-bundler/src/Bundler/__tests__/Bundler-test.js b/packages/metro-bundler/src/Bundler/__tests__/Bundler-test.js index f8b1639b..b4becd75 100644 --- a/packages/metro-bundler/src/Bundler/__tests__/Bundler-test.js +++ b/packages/metro-bundler/src/Bundler/__tests__/Bundler-test.js @@ -15,6 +15,7 @@ jest .setMock('uglify-js') .mock('image-size') .mock('fs') + .mock('os') .mock('assert') .mock('progress') .mock('../../node-haste') @@ -31,6 +32,7 @@ var Resolver = require('../../Resolver'); var defaults = require('../../../defaults'); var sizeOf = require('image-size'); var fs = require('fs'); +const os = require('os'); var commonOptions = { allowBundleUpdates: false, @@ -76,6 +78,8 @@ describe('Bundler', function() { var projectRoots; beforeEach(function() { + os.cpus.mockReturnValue({length: 1}); + getDependencies = jest.fn(); getModuleSystemDependencies = jest.fn(); projectRoots = ['/root']; @@ -348,5 +352,16 @@ describe('Bundler', function() { '/root/img/new_image2@3x.png', ])); }); + + it('return correct number of workers', () => { + os.cpus.mockReturnValue({length: 1}); + expect(Bundler.getMaxWorkerCount()).toBe(1); + os.cpus.mockReturnValue({length: 8}); + expect(Bundler.getMaxWorkerCount()).toBe(6); + os.cpus.mockReturnValue({length: 24}); + expect(Bundler.getMaxWorkerCount()).toBe(14); + process.env.REACT_NATIVE_MAX_WORKERS = 5; + expect(Bundler.getMaxWorkerCount()).toBe(5); + }); }); }); diff --git a/packages/metro-bundler/src/Bundler/index.js b/packages/metro-bundler/src/Bundler/index.js index b4dcf647..7066db33 100644 --- a/packages/metro-bundler/src/Bundler/index.js +++ b/packages/metro-bundler/src/Bundler/index.js @@ -25,6 +25,8 @@ const imageSize = require('image-size'); const path = require('path'); const denodeify = require('denodeify'); const defaults = require('../../defaults'); +const os = require('os'); +const invariant = require('fbjs/lib/invariant'); const { sep: pathSeparator, @@ -163,8 +165,10 @@ class Bundler { cacheKey: transformCacheKey, }); + const maxWorkerCount = Bundler.getMaxWorkerCount(); + /* $FlowFixMe: in practice it's always here. */ - this._transformer = new Transformer(opts.transformModulePath); + this._transformer = new Transformer(opts.transformModulePath, maxWorkerCount); const getTransformCacheKey = (src, filename, options) => { return transformCacheKey + getCacheKey(src, filename, options); @@ -178,6 +182,7 @@ class Bundler { getTransformCacheKey, globalTransformCache: opts.globalTransformCache, hasteImpl: opts.hasteImpl, + maxWorkerCount, minifyCode: this._transformer.minify, moduleFormat: opts.moduleFormat, platforms: new Set(opts.platforms), @@ -780,6 +785,26 @@ class Bundler { getResolver(): Promise { return this._resolverPromise; } + + /** + * Unless overriden, we use a diminishing amount of workers per core, because + * using more and more of them does not scale much. Ex. 6 workers for 8 + * cores, or 14 workers for 24 cores. + */ + static getMaxWorkerCount() { + const cores = os.cpus().length; + const envStr = process.env.REACT_NATIVE_MAX_WORKERS; + if (envStr == null) { + return Math.max(1, Math.ceil(cores * (0.5 + 0.5 * Math.exp(-cores * 0.07)) - 1)); + } + const envCount = parseInt(process.env.REACT_NATIVE_MAX_WORKERS, 10); + invariant( + Number.isInteger(envCount), + 'environment variable `REACT_NATIVE_MAX_WORKERS` must be a valid integer', + ); + return Math.min(cores, envCount); + } + } function getPathRelativeToRoot(roots, absPath) { diff --git a/packages/metro-bundler/src/JSTransformer/index.js b/packages/metro-bundler/src/JSTransformer/index.js index 393eb245..6cc7e56e 100644 --- a/packages/metro-bundler/src/JSTransformer/index.js +++ b/packages/metro-bundler/src/JSTransformer/index.js @@ -16,7 +16,6 @@ const Logger = require('../Logger'); const debug = require('debug')('RNP:JStransformer'); const denodeify = require('denodeify'); const invariant = require('fbjs/lib/invariant'); -const os = require('os'); const path = require('path'); const util = require('util'); const workerFarm = require('worker-farm'); @@ -35,24 +34,7 @@ const TRANSFORM_TIMEOUT_INTERVAL = 301000; // How may times can we tolerate failures from the worker. const MAX_RETRIES = 2; -const maxConcurrentWorkers = ((cores, override) => { - if (override) { - return Math.min(cores, override); - } - - if (cores < 3) { - return cores; - } - if (cores < 8) { - return Math.floor(cores * 0.75); - } - if (cores < 24) { - return Math.floor(3 / 8 * cores + 3); // between cores *.75 and cores / 2 - } - return cores / 2; -})(os.cpus().length, parseInt(process.env.REACT_NATIVE_MAX_WORKERS, 10)); - -function makeFarm(worker, methods, timeout) { +function makeFarm(worker, methods, timeout, maxConcurrentWorkers) { return workerFarm( { autoStart: true, @@ -83,7 +65,7 @@ class Transformer { sourceMap: SourceMap, ) => Promise<{code: string, map: SourceMap}>; - constructor(transformModulePath: string) { + constructor(transformModulePath: string, maxWorkerCount: number) { invariant(path.isAbsolute(transformModulePath), 'transform module path should be absolute'); this._transformModulePath = transformModulePath; @@ -91,6 +73,7 @@ class Transformer { require.resolve('./worker'), ['minify', 'transformAndExtractDependencies'], TRANSFORM_TIMEOUT_INTERVAL, + maxWorkerCount, ); this._transform = denodeify(this._workers.transformAndExtractDependencies); this.minify = denodeify(this._workers.minify); diff --git a/packages/metro-bundler/src/Resolver/index.js b/packages/metro-bundler/src/Resolver/index.js index 31d53c9d..e73706bf 100644 --- a/packages/metro-bundler/src/Resolver/index.js +++ b/packages/metro-bundler/src/Resolver/index.js @@ -37,6 +37,7 @@ type Options = { getTransformCacheKey: GetTransformCacheKey, globalTransformCache: ?GlobalTransformCache, hasteImpl?: HasteImpl, + maxWorkerCount: number, minifyCode: MinifyCode, platforms: Set, polyfillModuleNames?: Array, diff --git a/packages/metro-bundler/src/node-haste/__tests__/DependencyGraph-test.js b/packages/metro-bundler/src/node-haste/__tests__/DependencyGraph-test.js index 3c5aca6f..4a60d0b5 100644 --- a/packages/metro-bundler/src/node-haste/__tests__/DependencyGraph-test.js +++ b/packages/metro-bundler/src/node-haste/__tests__/DependencyGraph-test.js @@ -112,7 +112,7 @@ describe('DependencyGraph', function() { platforms: new Set(['ios', 'android']), useWatchman: false, ignoreFilePath: () => false, - maxWorkers: 1, + maxWorkerCount: 1, moduleOptions: {cacheTransformResults: true}, resetCache: true, transformCode: (module, sourceCode, transformOptions) => { diff --git a/packages/metro-bundler/src/node-haste/index.js b/packages/metro-bundler/src/node-haste/index.js index 25af7db6..6af6a190 100644 --- a/packages/metro-bundler/src/node-haste/index.js +++ b/packages/metro-bundler/src/node-haste/index.js @@ -24,8 +24,8 @@ const fs = require('fs'); const getAssetDataFromName = require('./lib/getAssetDataFromName'); const getInverseDependencies = require('./lib/getInverseDependencies'); const getPlatformExtension = require('./lib/getPlatformExtension'); +const invariant = require('fbjs/lib/invariant'); const isAbsolutePath = require('absolute-path'); -const os = require('os'); const path = require('path'); const replacePatterns = require('./lib/replacePatterns'); const util = require('util'); @@ -58,7 +58,7 @@ type Options = { getTransformCacheKey: GetTransformCacheKey, globalTransformCache: ?GlobalTransformCache, ignoreFilePath: (filePath: string) => boolean, - maxWorkers: ?number, + maxWorkerCount: number, moduleOptions: ModuleOptions, platforms: Set, preferNativePlatform: boolean, @@ -87,6 +87,7 @@ class DependencyGraph extends EventEmitter { initialModuleMap: ModuleMap, }) { super(); + invariant(config.opts.maxWorkerCount >= 1, 'worker count must be greater or equal to 1'); this._opts = config.opts; this._haste = config.haste; this._hasteFS = config.initialHasteFS; @@ -97,12 +98,11 @@ class DependencyGraph extends EventEmitter { } static _createHaste(opts: Options): JestHasteMap { - const mw = opts.maxWorkers; return new JestHasteMap({ extensions: opts.extensions.concat(opts.assetExts), forceNodeFilesystemAPI: opts.forceNodeFilesystemAPI, ignorePattern: {test: opts.ignoreFilePath}, - maxWorkers: typeof mw === 'number' && mw >= 1 ? mw : getMaxWorkers(), + maxWorkers: opts.maxWorkerCount, mocksPattern: '', name: 'react-native-packager', platforms: Array.from(opts.platforms), @@ -306,28 +306,4 @@ function NotFoundError() { } util.inherits(NotFoundError, Error); -function getMaxWorkers() { - const cores = os.cpus().length; - - if (cores <= 1) { - // oh well... - return 1; - } - if (cores <= 4) { - // don't starve the CPU while still reading reasonably rapidly - return cores - 1; - } - if (cores <= 8) { - // empirical testing showed massive diminishing returns when going over - // 4 or 5 workers on 8-core machines - return Math.floor(cores * 0.75) - 1; - } - - // pretty much guesswork - if (cores < 24) { - return Math.floor(3 / 8 * cores + 3); - } - return cores / 2; -} - module.exports = DependencyGraph;