From 80adeec3b0f2083aecb5170fcb7ca2a4e6bd28da Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 29 Oct 2022 15:18:52 -0700 Subject: [PATCH 1/2] Improve performance of incremental bundling --- .../core/src/requests/AssetGraphRequest.js | 16 +- .../core/src/requests/BundleGraphRequest.js | 241 ++++++++---------- .../core/src/requests/ParcelBuildRequest.js | 23 +- .../test/incremental-bundling.js | 53 ++-- packages/core/parcel/src/cli.js | 7 +- 5 files changed, 152 insertions(+), 188 deletions(-) diff --git a/packages/core/core/src/requests/AssetGraphRequest.js b/packages/core/core/src/requests/AssetGraphRequest.js index 2b0d55073d5..cc704a10b7d 100644 --- a/packages/core/core/src/requests/AssetGraphRequest.js +++ b/packages/core/core/src/requests/AssetGraphRequest.js @@ -49,11 +49,7 @@ type AssetGraphRequestInput = {| requestedAssetIds?: Set, |}; -type AssetGraphRequestResult = AssetGraphBuilderResult & {| - previousAssetGraphHash: ?string, -|}; - -type AssetGraphBuilderResult = {| +type AssetGraphRequestResult = {| assetGraph: AssetGraph, changedAssets: Map, assetRequests: Array, @@ -80,7 +76,6 @@ export default function createAssetGraphRequest( run: async input => { let prevResult = await input.api.getPreviousResult(); - let previousAssetGraphHash = prevResult?.assetGraph.getHash(); let builder = new AssetGraphBuilder(input, prevResult); let assetGraphRequest = await await builder.build(); @@ -93,10 +88,7 @@ export default function createAssetGraphRequest( assetGraphRequest.assetGraph.safeToIncrementallyBundle = false; } - return { - ...assetGraphRequest, - previousAssetGraphHash, - }; + return assetGraphRequest; }, input, }; @@ -124,7 +116,7 @@ export class AssetGraphBuilder { constructor( {input, api, options}: RunInput, - prevResult: ?AssetGraphBuilderResult, + prevResult: ?AssetGraphRequestResult, ) { let { entries, @@ -154,7 +146,7 @@ export class AssetGraphBuilder { this.queue = new PromiseQueue(); } - async build(): Promise { + async build(): Promise { let errors = []; let rootNodeId = nullthrows( this.assetGraph.rootNodeId, diff --git a/packages/core/core/src/requests/BundleGraphRequest.js b/packages/core/core/src/requests/BundleGraphRequest.js index 248628450f5..8b45482be39 100644 --- a/packages/core/core/src/requests/BundleGraphRequest.js +++ b/packages/core/core/src/requests/BundleGraphRequest.js @@ -6,12 +6,14 @@ import type ParcelConfig, {LoadedPlugin} from '../ParcelConfig'; import type {StaticRunOpts, RunAPI} from '../RequestTracker'; import type { Asset, + AssetGroup, Bundle as InternalBundle, Config, DevDepRequest, ParcelOptions, } from '../types'; import type {ConfigAndCachePath} from './ParcelConfigRequest'; +import type {AbortSignal} from 'abortcontroller-polyfill/dist/cjs-ponyfill'; import invariant from 'assert'; import assert from 'assert'; @@ -29,8 +31,8 @@ import {unique} from '@parcel/utils'; import {hashString} from '@parcel/hash'; import PluginOptions from '../public/PluginOptions'; import applyRuntimes from '../applyRuntimes'; -import {PARCEL_VERSION} from '../constants'; -import {optionsProxy} from '../utils'; +import {PARCEL_VERSION, OPTION_CHANGE} from '../constants'; +import {assertSignalNotAborted, optionsProxy} from '../utils'; import createParcelConfigRequest, { getCachedParcelConfig, } from './ParcelConfigRequest'; @@ -40,31 +42,28 @@ import { invalidateDevDeps, runDevDepRequest, } from './DevDepRequest'; -import {getInvalidationHash} from '../assetUtils'; import {createConfig} from '../InternalConfig'; import { loadPluginConfig, runConfigRequest, - getConfigHash, type PluginWithLoadConfig, } from './ConfigRequest'; -import {cacheSerializedObject, deserializeToCache} from '../serializer'; +import {cacheSerializedObject} from '../serializer'; import { joinProjectPath, fromProjectPathRelative, toProjectPathUnsafe, } from '../projectPath'; +import createAssetGraphRequest from './AssetGraphRequest'; type BundleGraphRequestInput = {| - assetGraph: AssetGraph, - changedAssets: Map, - previousAssetGraphHash: ?string, + requestedAssetIds: Set, + signal?: AbortSignal, optionsRef: SharedReference, |}; type BundleGraphRequestResult = {| bundleGraph: InternalBundleGraph, - bundlerHash: string, |}; type RunInput = {| @@ -74,8 +73,8 @@ type RunInput = {| type BundleGraphResult = {| bundleGraph: InternalBundleGraph, - bundlerHash: string, changedAssets: Map, + assetRequests: Array, |}; type BundleGraphRequest = {| @@ -90,24 +89,70 @@ export default function createBundleGraphRequest( ): BundleGraphRequest { return { type: 'bundle_graph_request', - id: 'BundleGraph:' + input.assetGraph.getHash(), + id: 'BundleGraph', run: async input => { + let {options, api, invalidateReason} = input; + let {optionsRef, requestedAssetIds, signal} = input.input; + let request = createAssetGraphRequest({ + name: 'Main', + entries: options.entries, + optionsRef, + shouldBuildLazily: options.shouldBuildLazily, + requestedAssetIds, + }); + let {assetGraph, changedAssets, assetRequests} = await api.runRequest( + request, + { + force: options.shouldBuildLazily && requestedAssetIds.size > 0, + }, + ); + + assertSignalNotAborted(signal); + + // If any subrequests are invalid (e.g. dev dep requests or config requests), + // bail on incremental bundling. We also need to invalidate for option changes, + // which are hoisted to direct invalidations on the bundle graph request. + let subRequestsInvalid = + Boolean(invalidateReason & OPTION_CHANGE) || + input.api + .getSubRequests() + .some(req => !input.api.canSkipSubrequest(req.id)); + + if (subRequestsInvalid) { + assetGraph.safeToIncrementallyBundle = false; + } + let configResult = nullthrows( await input.api.runRequest( createParcelConfigRequest(), ), ); - let parcelConfig = getCachedParcelConfig(configResult, input.options); + assertSignalNotAborted(signal); + + let parcelConfig = getCachedParcelConfig(configResult, input.options); let {devDeps, invalidDevDeps} = await getDevDepRequests(input.api); invalidateDevDeps(invalidDevDeps, input.options, parcelConfig); let builder = new BundlerRunner(input, parcelConfig, devDeps); - return builder.bundle({ - graph: input.input.assetGraph, - previousAssetGraphHash: input.input.previousAssetGraphHash, - changedAssets: input.input.changedAssets, + let res: BundleGraphResult = await builder.bundle({ + graph: assetGraph, + changedAssets: changedAssets, + assetRequests, }); + + for (let [id, asset] of changedAssets) { + res.changedAssets.set(id, asset); + } + + // $FlowFixMe Added in Flow 0.121.0 upgrade in #4381 (Windows only) + dumpGraphToGraphViz( + res.bundleGraph._graph, + 'BundleGraph', + bundleGraphEdgeTypes, + ); + + return res; }, input, }; @@ -122,6 +167,7 @@ class BundlerRunner { previousDevDeps: Map; devDepRequests: Map; configs: Map; + cacheKey: string; constructor( {input, api, options}: RunInput, @@ -138,6 +184,11 @@ class BundlerRunner { this.pluginOptions = new PluginOptions( optionsProxy(this.options, api.invalidateOnOptionChange), ); + this.cacheKey = hashString( + `${PARCEL_VERSION}:BundleGraph:${JSON.stringify(options.entries) ?? ''}${ + options.mode + }`, + ); } async loadConfigs() { @@ -185,12 +236,12 @@ class BundlerRunner { async bundle({ graph, - previousAssetGraphHash, changedAssets, + assetRequests, }: {| graph: AssetGraph, - previousAssetGraphHash: ?string, changedAssets: Map, + assetRequests: Array, |}): Promise { report({ type: 'buildProgress', @@ -202,52 +253,16 @@ class BundlerRunner { let plugin = await this.config.getBundler(); let {plugin: bundler, name, resolveFrom} = plugin; - let {cacheKey, bundlerHash} = await this.getHashes(graph); - - // Check if the cacheKey matches the one already stored in the graph. - // This can save time deserializing from cache if the graph is already in memory. - // This will only happen in watch mode. In this case, serialization will occur once - // when sending the bundle graph to workers, and again on shutdown when writing to cache. - let previousResult = await this.api.getPreviousResult(cacheKey); - if (previousResult != null) { - // No need to call api.storeResult here because it's already the request result. - return previousResult; - } - - // Otherwise, check the cache in case the cache key has already been written to disk. - if (!this.options.shouldDisableCache) { - let cached = await this.options.cache.getBuffer(cacheKey); - if (cached != null) { - // Deserialize, and store the original buffer in an in memory cache so we avoid - // re-serializing it when sending to workers, and in build mode, when writing to cache on shutdown. - let graph = deserializeToCache(cached); - this.api.storeResult( - { - bundleGraph: graph, - changedAssets: new Map(), - }, - cacheKey, - ); - return graph; - } - } - // if a previous asset graph hash is passed in, check if the bundle graph is also available let previousBundleGraphResult: ?BundleGraphRequestResult; - if (graph.safeToIncrementallyBundle && previousAssetGraphHash != null) { + if (graph.safeToIncrementallyBundle) { try { - previousBundleGraphResult = - await this.api.getRequestResult( - 'BundleGraph:' + previousAssetGraphHash, - ); + previousBundleGraphResult = await this.api.getPreviousResult(); } catch { // if the bundle graph had an error or was removed, don't fail the build } } - if ( - previousBundleGraphResult == null || - previousBundleGraphResult?.bundlerHash !== bundlerHash - ) { + if (previousBundleGraphResult == null) { graph.safeToIncrementallyBundle = false; } @@ -256,8 +271,8 @@ class BundlerRunner { let logger = new PluginLogger({origin: name}); try { - if (graph.safeToIncrementallyBundle) { - internalBundleGraph = nullthrows(previousBundleGraphResult).bundleGraph; + if (previousBundleGraphResult) { + internalBundleGraph = previousBundleGraphResult.bundleGraph; for (let changedAsset of changedAssets.values()) { internalBundleGraph.updateAsset(changedAsset); } @@ -306,6 +321,18 @@ class BundlerRunner { ); } } + + // Add dev dependency for the bundler. This must be done AFTER running it due to + // the potential for lazy require() that aren't executed until the request runs. + let devDepRequest = await createDevDependency( + { + specifier: name, + resolveFrom, + }, + this.previousDevDeps, + this.options, + ); + await this.runDevDepRequest(devDepRequest); } } catch (e) { throw new ThrowableDiagnostic({ @@ -323,31 +350,28 @@ class BundlerRunner { ); } - // Add dev dependency for the bundler. This must be done AFTER running it due to - // the potential for lazy require() that aren't executed until the request runs. - let devDepRequest = await createDevDependency( - { - specifier: name, - resolveFrom, - }, - this.previousDevDeps, - this.options, - ); - await this.runDevDepRequest(devDepRequest); + let changedRuntimes = new Map(); + if (!previousBundleGraphResult) { + await this.nameBundles(internalBundleGraph); - await this.nameBundles(internalBundleGraph); + changedRuntimes = await applyRuntimes({ + bundleGraph: internalBundleGraph, + api: this.api, + config: this.config, + options: this.options, + optionsRef: this.optionsRef, + pluginOptions: this.pluginOptions, + previousDevDeps: this.previousDevDeps, + devDepRequests: this.devDepRequests, + configs: this.configs, + }); - let changedRuntimes = await applyRuntimes({ - bundleGraph: internalBundleGraph, - api: this.api, - config: this.config, - options: this.options, - optionsRef: this.optionsRef, - pluginOptions: this.pluginOptions, - previousDevDeps: this.previousDevDeps, - devDepRequests: this.devDepRequests, - configs: this.configs, - }); + // Store the serialized bundle graph in an in memory cache so that we avoid serializing it + // many times to send to each worker, and in build mode, when writing to cache on shutdown. + // Also, pre-compute the hashes for each bundle so they are only computed once and shared between workers. + internalBundleGraph.getBundleGraphHash(); + cacheSerializedObject(internalBundleGraph); + } await dumpGraphToGraphViz( // $FlowFixMe @@ -356,66 +380,19 @@ class BundlerRunner { bundleGraphEdgeTypes, ); - // Store the serialized bundle graph in an in memory cache so that we avoid serializing it - // many times to send to each worker, and in build mode, when writing to cache on shutdown. - // Also, pre-compute the hashes for each bundle so they are only computed once and shared between workers. - internalBundleGraph.getBundleGraphHash(); - cacheSerializedObject(internalBundleGraph); - - // Recompute the cache key to account for new dev dependencies and invalidations. - let {cacheKey: updatedCacheKey} = await this.getHashes(graph); this.api.storeResult( { - bundlerHash, bundleGraph: internalBundleGraph, changedAssets: new Map(), + assetRequests: [], }, - updatedCacheKey, + this.cacheKey, ); return { bundleGraph: internalBundleGraph, - bundlerHash, changedAssets: changedRuntimes, - }; - } - - async getHashes(assetGraph: AssetGraph): Promise<{| - cacheKey: string, - bundlerHash: string, - |}> { - // BundleGraphRequest needs hashes based on content (for quick retrieval) - // and not-based on content (determine if the environment / config - // changes that violate incremental bundling). - let configs = ( - await Promise.all( - [...this.configs].map(([pluginName, config]) => - getConfigHash(config, pluginName, this.options), - ), - ) - ).join(''); - - let devDepRequests = [...this.devDepRequests.values()] - .map(d => d.hash) - .join(''); - - let invalidations = await getInvalidationHash( - this.api.getInvalidations(), - this.options, - ); - - let plugin = await this.config.getBundler(); - - return { - cacheKey: hashString( - PARCEL_VERSION + - assetGraph.getHash() + - configs + - devDepRequests + - invalidations + - this.options.mode, - ), - bundlerHash: hashString(PARCEL_VERSION + plugin.name + configs), + assetRequests, }; } diff --git a/packages/core/core/src/requests/ParcelBuildRequest.js b/packages/core/core/src/requests/ParcelBuildRequest.js index f52a02c119e..f6ffe91676d 100644 --- a/packages/core/core/src/requests/ParcelBuildRequest.js +++ b/packages/core/core/src/requests/ParcelBuildRequest.js @@ -9,7 +9,6 @@ import type {StaticRunOpts} from '../RequestTracker'; import type {Asset, AssetGroup, PackagedBundleInfo} from '../types'; import type BundleGraph from '../BundleGraph'; -import createAssetGraphRequest from './AssetGraphRequest'; import createBundleGraphRequest from './BundleGraphRequest'; import createWriteBundlesRequest from './WriteBundlesRequest'; import {assertSignalNotAborted} from '../utils'; @@ -54,31 +53,17 @@ export default function createParcelBuildRequest( async function run({input, api, options}: RunInput) { let {optionsRef, requestedAssetIds, signal} = input; - let request = createAssetGraphRequest({ - name: 'Main', - entries: options.entries, - optionsRef, - shouldBuildLazily: options.shouldBuildLazily, - requestedAssetIds, - }); - let {assetGraph, changedAssets, assetRequests, previousAssetGraphHash} = - await api.runRequest(request, { - force: options.shouldBuildLazily && requestedAssetIds.size > 0, - }); let bundleGraphRequest = createBundleGraphRequest({ - assetGraph, - previousAssetGraphHash, - changedAssets, optionsRef, + requestedAssetIds, + signal, }); - let {bundleGraph, changedAssets: changedRuntimeAssets} = await api.runRequest( + let {bundleGraph, changedAssets, assetRequests} = await api.runRequest( bundleGraphRequest, + {force: options.shouldBuildLazily && requestedAssetIds.size > 0}, ); - for (let [id, asset] of changedRuntimeAssets) { - changedAssets.set(id, asset); - } // $FlowFixMe Added in Flow 0.121.0 upgrade in #4381 (Windows only) dumpGraphToGraphViz(bundleGraph._graph, 'BundleGraph', bundleGraphEdgeTypes); diff --git a/packages/core/integration-tests/test/incremental-bundling.js b/packages/core/integration-tests/test/incremental-bundling.js index ac77b3e5cc8..daafbd578ad 100644 --- a/packages/core/integration-tests/test/incremental-bundling.js +++ b/packages/core/integration-tests/test/incremental-bundling.js @@ -1,26 +1,23 @@ // @flow strict-local -import {bundler, getNextBuildSuccess, overlayFS, run} from '@parcel/test-utils'; +import { + bundler, + getNextBuildSuccess, + inputFS, + overlayFS, + run, +} from '@parcel/test-utils'; import assert from 'assert'; import path from 'path'; import sinon from 'sinon'; -import Bundler from '@parcel/bundler-default'; -import ExperimentalBundler from '@parcel/bundler-experimental'; +import {NodePackageManager} from '@parcel/package-manager'; import {type Asset} from '@parcel/types'; -// $FlowFixMe[untyped-import] -import CustomBundler from './integration/incremental-bundling/node_modules/parcel-bundler-test'; const CONFIG = Symbol.for('parcel-plugin-config'); +let packageManager = new NodePackageManager(inputFS, '/'); describe('incremental bundling', function () { - let defaultBundlerSpy = - process.env.PARCEL_TEST_EXPERIMENTAL_BUNDLER != null - ? // $FlowFixMe[prop-missing] - sinon.spy(ExperimentalBundler[CONFIG], 'bundle') - : // $FlowFixMe[prop-missing] - sinon.spy(Bundler[CONFIG], 'bundle'); - let customBundlerSpy = sinon.spy(CustomBundler[CONFIG], 'bundle'); - + let defaultBundlerSpy, customBundlerSpy; let assertChangedAssets = (actual: number, expected: number) => { assert.equal( actual, @@ -40,12 +37,29 @@ describe('incremental bundling', function () { let getChangedAssetsBeforeRuntimes = (changedAssets: Array) => { return changedAssets.filter(a => !a.filePath.includes('runtime')); }; - beforeEach(() => { - defaultBundlerSpy.resetHistory(); - customBundlerSpy.resetHistory(); + beforeEach(async () => { + let Bundler = ( + await packageManager.require('@parcel/bundler-default', __filename) + ).default; + let ExperimentalBundler = ( + await packageManager.require('@parcel/bundler-experimental', __filename) + ).default; + let CustomBundler = await packageManager.require( + './integration/incremental-bundling/node_modules/parcel-bundler-test', + __filename, + ); + + defaultBundlerSpy = + process.env.PARCEL_TEST_EXPERIMENTAL_BUNDLER != null + ? // $FlowFixMe[prop-missing] + sinon.spy(ExperimentalBundler[CONFIG], 'bundle') + : // $FlowFixMe[prop-missing] + sinon.spy(Bundler[CONFIG], 'bundle'); + + customBundlerSpy = sinon.spy(CustomBundler[CONFIG], 'bundle'); }); - after(() => { + afterEach(() => { defaultBundlerSpy.restore(); customBundlerSpy.restore(); }); @@ -781,8 +795,7 @@ console.log('index.js');`, // should contain all the assets assertChangedAssets(event.changedAssets.size, 3); - // the default bundler was only called once - assertTimesBundled(defaultBundlerSpy.callCount, 1); + assertTimesBundled(defaultBundlerSpy.callCount, 2); let result = await b.run(); let bundles = result.bundleGraph.getBundles(); @@ -827,7 +840,7 @@ console.log('index.js');`, // should contain all the assets let assets = Array.from(event.changedAssets.values()); assertChangedAssets(getChangedAssetsBeforeRuntimes(assets).length, 3); - assertTimesBundled(defaultBundlerSpy.callCount, 1); + assertTimesBundled(defaultBundlerSpy.callCount, 2); let result = await b.run(); let res = await run(result.bundleGraph, null, {require: false}); diff --git a/packages/core/parcel/src/cli.js b/packages/core/parcel/src/cli.js index f0665c3f208..73a14fc4572 100755 --- a/packages/core/parcel/src/cli.js +++ b/packages/core/parcel/src/cli.js @@ -143,10 +143,6 @@ let serve = program '--lazy', 'Build async bundles on demand, when requested in the browser', ) - .option( - '--no-incremental', - 'Disables bundling skipping. Default builds are faster when modifying a file without adding or removing dependencies', - ) .action(runCommand); applyOptions(serve, hmrOptions); @@ -482,7 +478,8 @@ async function normalizeOptions( logLevel: command.logLevel, shouldProfile: command.profile, shouldBuildLazily: command.lazy, - shouldBundleIncrementally: command.incremental ? true : false, // default is now true, turn off with "--no-incremental" + shouldBundleIncrementally: + process.env.PARCEL_INCREMENTAL_BUNDLING === 'false' ? false : true, detailedReport: command.detailedReport != null ? { From dd1711f9edff47b0973b90307157ec6545c2006c Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sat, 29 Oct 2022 17:44:21 -0700 Subject: [PATCH 2/2] fixes --- packages/core/core/src/requests/BundleGraphRequest.js | 2 +- .../core/integration-tests/test/incremental-bundling.js | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/core/core/src/requests/BundleGraphRequest.js b/packages/core/core/src/requests/BundleGraphRequest.js index 8b45482be39..c9f14e540de 100644 --- a/packages/core/core/src/requests/BundleGraphRequest.js +++ b/packages/core/core/src/requests/BundleGraphRequest.js @@ -145,8 +145,8 @@ export default function createBundleGraphRequest( res.changedAssets.set(id, asset); } - // $FlowFixMe Added in Flow 0.121.0 upgrade in #4381 (Windows only) dumpGraphToGraphViz( + // $FlowFixMe Added in Flow 0.121.0 upgrade in #4381 (Windows only) res.bundleGraph._graph, 'BundleGraph', bundleGraphEdgeTypes, diff --git a/packages/core/integration-tests/test/incremental-bundling.js b/packages/core/integration-tests/test/incremental-bundling.js index daafbd578ad..24540652ef7 100644 --- a/packages/core/integration-tests/test/incremental-bundling.js +++ b/packages/core/integration-tests/test/incremental-bundling.js @@ -787,6 +787,10 @@ console.log('index.js');`, path.join(fixture, '.parcelrc'), JSON.stringify({ extends: '@parcel/config-default', + bundler: + process.env.PARCEL_TEST_EXPERIMENTAL_BUNDLER != null + ? '@parcel/bundler-experimental' + : undefined, namers: ['parcel-namer-test'], }), ); @@ -831,6 +835,10 @@ console.log('index.js');`, path.join(fixture, '.parcelrc'), JSON.stringify({ extends: '@parcel/config-default', + bundler: + process.env.PARCEL_TEST_EXPERIMENTAL_BUNDLER != null + ? '@parcel/bundler-experimental' + : undefined, runtimes: ['parcel-runtime-test'], }), );