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

Optimize bundler performance #9266

Merged
merged 10 commits into from
Sep 30, 2023
760 changes: 382 additions & 378 deletions packages/bundlers/default/src/DefaultBundler.js

Large diffs are not rendered by default.

9 changes: 5 additions & 4 deletions packages/core/core/src/BundleGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,8 @@ export default class BundleGraph {
: null;
invariant(assetGraphRootNode != null && assetGraphRootNode.type === 'root');

for (let [nodeId, node] of assetGraph.nodes) {
if (node.type === 'asset') {
for (let [nodeId, node] of assetGraph.nodes.entries()) {
if (node != null && node.type === 'asset') {
let {id: assetId} = node.value;
// Generate a new, short public id for this asset to use.
// If one already exists, use it.
Expand All @@ -187,7 +187,7 @@ export default class BundleGraph {
publicIdByAssetId.set(assetId, publicId);
assetPublicIds.add(publicId);
}
} else if (node.type === 'asset_group') {
} else if (node != null && node.type === 'asset_group') {
assetGroupIds.set(nodeId, assetGraph.getNodeIdsConnectedFrom(nodeId));
}
}
Expand Down Expand Up @@ -2011,7 +2011,8 @@ export default class BundleGraph {

merge(other: BundleGraph) {
let otherGraphIdToThisNodeId = new Map<NodeId, NodeId>();
for (let [otherNodeId, otherNode] of other._graph.nodes) {
for (let [otherNodeId, otherNode] of other._graph.nodes.entries()) {
if (!otherNode) continue;
if (this._graph.hasContentKey(otherNode.id)) {
let existingNodeId = this._graph.getNodeIdByContentKey(otherNode.id);
otherGraphIdToThisNodeId.set(otherNodeId, existingNodeId);
Expand Down
8 changes: 4 additions & 4 deletions packages/core/core/src/RequestTracker.js
Original file line number Diff line number Diff line change
Expand Up @@ -724,8 +724,8 @@ export class RequestGraph extends ContentGraph<
// this means the project root was moved and we need to
// re-run all requests.
if (type === 'create' && filePath === '') {
for (let [id, node] of this.nodes) {
if (node.type === 'request') {
for (let [id, node] of this.nodes.entries()) {
if (node?.type === 'request') {
this.invalidNodeIds.add(id);
}
}
Expand Down Expand Up @@ -1087,8 +1087,8 @@ export default class RequestTracker {
}

let promises = [];
for (let [, node] of this.graph.nodes) {
if (node.type !== 'request') {
for (let node of this.graph.nodes) {
if (!node || node.type !== 'request') {
continue;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/core/core/src/SymbolPropagation.js
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,7 @@ function propagateSymbolsUp(
let runFullPass =
// If there are n nodes in the graph, then the asset count is approximately
// n/6 (for every asset, there are ~4 dependencies and ~1 asset_group).
assetGraph.nodes.size * (1 / 6) * 0.5 <
assetGraph.nodes.length * (1 / 6) * 0.5 <
changedDepsUsedSymbolsUpDirtyDownAssets.size;

let dirtyDeps;
Expand Down
4 changes: 3 additions & 1 deletion packages/core/core/src/dumpGraphToGraphViz.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ export default async function dumpGraphToGraphViz(
const graphviz = require('graphviz');
const tempy = require('tempy');
let g = graphviz.digraph('G');
for (let [id, node] of graph.nodes) {
// $FlowFixMe
for (let [id, node] of graph.nodes.entries()) {
if (node == null) continue;
let n = g.addNode(nodeId(id));
// $FlowFixMe default is fine. Not every type needs to be in the map.
n.set('color', COLORS[node.type || 'default']);
Expand Down
10 changes: 8 additions & 2 deletions packages/core/core/src/public/Asset.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import type {Asset as AssetValue, ParcelOptions} from '../types';

import nullthrows from 'nullthrows';
import Environment from './Environment';
import Dependency from './Dependency';
import {getPublicDependency} from './Dependency';
import {AssetSymbols, MutableAssetSymbols} from './Symbols';
import UncommittedAsset from '../UncommittedAsset';
import CommittedAsset from '../CommittedAsset';
Expand Down Expand Up @@ -162,7 +162,7 @@ class BaseAsset {
getDependencies(): $ReadOnlyArray<IDependency> {
return this.#asset
.getDependencies()
.map(dep => new Dependency(dep, this.#asset.options));
.map(dep => getPublicDependency(dep, this.#asset.options));
}

getCode(): Promise<string> {
Expand Down Expand Up @@ -192,6 +192,7 @@ class BaseAsset {

export class Asset extends BaseAsset implements IAsset {
#asset /*: CommittedAsset | UncommittedAsset */;
#env /*: ?Environment */;

constructor(asset: CommittedAsset | UncommittedAsset): Asset {
let assetValueToAsset = asset.value.committed
Expand All @@ -208,6 +209,11 @@ export class Asset extends BaseAsset implements IAsset {
return this;
}

get env(): IEnvironment {
this.#env ??= new Environment(this.#asset.value.env, this.#asset.options);
return this.#env;
}

get stats(): Stats {
return this.#asset.value.stats;
}
Expand Down
7 changes: 5 additions & 2 deletions packages/core/core/src/public/Bundle.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ import {DefaultWeakMap} from '@parcel/utils';
import {assetToAssetValue, assetFromValue} from './Asset';
import {mapVisitor} from '@parcel/graph';
import Environment from './Environment';
import Dependency, {dependencyToInternalDependency} from './Dependency';
import {
dependencyToInternalDependency,
getPublicDependency,
} from './Dependency';
import Target from './Target';
import {BundleBehaviorNames} from '../types';
import {fromProjectPath} from '../projectPath';
Expand Down Expand Up @@ -179,7 +182,7 @@ export class Bundle implements IBundle {
} else if (node.type === 'dependency') {
return {
type: 'dependency',
value: new Dependency(node.value, this.#options),
value: getPublicDependency(node.value, this.#options),
};
}
}, visit),
Expand Down
38 changes: 25 additions & 13 deletions packages/core/core/src/public/BundleGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ import nullthrows from 'nullthrows';
import {mapVisitor} from '@parcel/graph';
import {assetFromValue, assetToAssetValue, Asset} from './Asset';
import {bundleToInternalBundle} from './Bundle';
import Dependency, {dependencyToInternalDependency} from './Dependency';
import Dependency, {
dependencyToInternalDependency,
getPublicDependency,
} from './Dependency';
import {targetToInternalTarget} from './Target';
import {fromInternalSourceLocation} from '../utils';
import BundleGroup, {bundleGroupToInternalBundleGroup} from './BundleGroup';
Expand Down Expand Up @@ -90,7 +93,7 @@ export default class BundleGraph<TBundle: IBundle>
getIncomingDependencies(asset: IAsset): Array<IDependency> {
return this.#graph
.getIncomingDependencies(assetToAssetValue(asset))
.map(dep => new Dependency(dep, this.#options));
.map(dep => getPublicDependency(dep, this.#options));
}

getAssetWithDependency(dep: IDependency): ?IAsset {
Expand Down Expand Up @@ -158,7 +161,7 @@ export default class BundleGraph<TBundle: IBundle>
getDependencies(asset: IAsset): Array<IDependency> {
return this.#graph
.getDependencies(assetToAssetValue(asset))
.map(dep => new Dependency(dep, this.#options));
.map(dep => getPublicDependency(dep, this.#options));
}

isAssetReachableFromBundle(asset: IAsset, bundle: IBundle): boolean {
Expand Down Expand Up @@ -256,18 +259,27 @@ export default class BundleGraph<TBundle: IBundle>
traverse<TContext>(
visit: GraphVisitor<BundleGraphTraversable, TContext>,
start?: ?IAsset,
opts?: ?{|skipUnusedDependencies?: boolean|},
): ?TContext {
return this.#graph.traverse(
mapVisitor(
node =>
node.type === 'asset'
? {type: 'asset', value: assetFromValue(node.value, this.#options)}
: {
type: 'dependency',
value: new Dependency(node.value, this.#options),
},
visit,
),
mapVisitor((node, actions) => {
// Skipping unused dependencies here is faster than doing an isDependencySkipped check inside the visitor
// because the node needs to be re-looked up by id from the hashmap.
if (
opts?.skipUnusedDependencies &&
node.type === 'dependency' &&
(node.hasDeferred || node.excluded)
) {
actions.skipChildren();
return null;
}
return node.type === 'asset'
? {type: 'asset', value: assetFromValue(node.value, this.#options)}
: {
type: 'dependency',
value: getPublicDependency(node.value, this.#options),
};
}, visit),
start ? assetToAssetValue(start) : undefined,
);
}
Expand Down
17 changes: 12 additions & 5 deletions packages/core/core/src/public/Dependency.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,23 @@ export function dependencyToInternalDependency(
return nullthrows(_dependencyToInternalDependency.get(dependency));
}

export function getPublicDependency(
dep: InternalDependency,
options: ParcelOptions,
): Dependency {
let existing = internalDependencyToDependency.get(dep);
if (existing != null) {
return existing;
}

return new Dependency(dep, options);
}

export default class Dependency implements IDependency {
#dep /*: InternalDependency */;
#options /*: ParcelOptions */;

constructor(dep: InternalDependency, options: ParcelOptions): Dependency {
let existing = internalDependencyToDependency.get(dep);
if (existing != null) {
return existing;
}

this.#dep = dep;
this.#options = options;
_dependencyToInternalDependency.set(this, dep);
Expand Down
2 changes: 1 addition & 1 deletion packages/core/core/src/requests/AssetGraphRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ export class AssetGraphBuilder {
throw errors[0];
}

if (this.assetGraph.nodes.size > 1) {
if (this.assetGraph.nodes.length > 1) {
await dumpGraphToGraphViz(
this.assetGraph,
'AssetGraph_' + this.name + '_before_prop',
Expand Down
4 changes: 2 additions & 2 deletions packages/core/core/src/requests/PathRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import nullthrows from 'nullthrows';
import path from 'path';
import {normalizePath} from '@parcel/utils';
import {report} from '../ReporterRunner';
import PublicDependency from '../public/Dependency';
import {getPublicDependency} from '../public/Dependency';
import PluginOptions from '../public/PluginOptions';
import ParcelConfig from '../ParcelConfig';
import createParcelConfigRequest, {
Expand Down Expand Up @@ -240,7 +240,7 @@ export class ResolverRunner {
}

async resolve(dependency: Dependency): Promise<ResolverResult> {
let dep = new PublicDependency(dependency, this.options);
let dep = getPublicDependency(dependency, this.options);
report({
type: 'buildProgress',
phase: 'resolving',
Expand Down
28 changes: 14 additions & 14 deletions packages/core/core/test/AssetGraph.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ describe('AssetGraph', () => {
nodeFromAssetGroup(req).id,
);
let dependencyNodeId = graph.getNodeIdByContentKey(dep.id);
assert(graph.nodes.has(assetGroupNodeId));
assert(graph.hasNode(assetGroupNodeId));
assert(graph.hasEdge(dependencyNodeId, assetGroupNodeId));

let req2 = {
Expand All @@ -272,13 +272,13 @@ describe('AssetGraph', () => {
let assetGroupNodeId2 = graph.getNodeIdByContentKey(
nodeFromAssetGroup(req2).id,
);
assert(!graph.nodes.has(assetGroupNodeId));
assert(graph.nodes.has(assetGroupNodeId2));
assert(!graph.hasNode(assetGroupNodeId));
assert(graph.hasNode(assetGroupNodeId2));
assert(graph.hasEdge(dependencyNodeId, assetGroupNodeId2));
assert(!graph.hasEdge(dependencyNodeId, assetGroupNodeId));

graph.resolveDependency(dep, req2, '5');
assert(graph.nodes.has(assetGroupNodeId2));
assert(graph.hasNode(assetGroupNodeId2));
assert(graph.hasEdge(dependencyNodeId, assetGroupNodeId2));
});

Expand Down Expand Up @@ -389,11 +389,11 @@ describe('AssetGraph', () => {
[...assets[1].dependencies.values()][0].id,
);

assert(graph.nodes.has(nodeId1));
assert(graph.nodes.has(nodeId2));
assert(graph.nodes.has(nodeId3));
assert(graph.nodes.has(dependencyNodeId1));
assert(graph.nodes.has(dependencyNodeId2));
assert(graph.hasNode(nodeId1));
assert(graph.hasNode(nodeId2));
assert(graph.hasNode(nodeId3));
assert(graph.hasNode(dependencyNodeId1));
assert(graph.hasNode(dependencyNodeId2));
assert(graph.hasEdge(assetGroupNode, nodeId1));
assert(graph.hasEdge(assetGroupNode, nodeId2));
assert(graph.hasEdge(assetGroupNode, nodeId3));
Expand Down Expand Up @@ -435,11 +435,11 @@ describe('AssetGraph', () => {

graph.resolveAssetGroup(req, assets2, '5');

assert(graph.nodes.has(nodeId1));
assert(graph.nodes.has(nodeId2));
assert(!graph.nodes.has(nodeId3));
assert(graph.nodes.has(dependencyNodeId1));
assert(!graph.nodes.has(dependencyNodeId2));
assert(graph.hasNode(nodeId1));
assert(graph.hasNode(nodeId2));
assert(!graph.hasNode(nodeId3));
assert(graph.hasNode(dependencyNodeId1));
assert(!graph.hasNode(dependencyNodeId2));
assert(graph.hasEdge(assetGroupNode, nodeId1));
assert(graph.hasEdge(assetGroupNode, nodeId2));
assert(!graph.hasEdge(assetGroupNode, nodeId3));
Expand Down
6 changes: 3 additions & 3 deletions packages/core/core/test/PublicDependency.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import assert from 'assert';
import {createEnvironment} from '../src/Environment';
import {createDependency} from '../src/Dependency';
import Dependency from '../src/public/Dependency';
import {getPublicDependency} from '../src/public/Dependency';
import {DEFAULT_OPTIONS} from './test-utils';

describe('Public Dependency', () => {
Expand All @@ -15,8 +15,8 @@ describe('Public Dependency', () => {
});

assert.equal(
new Dependency(internalDependency, DEFAULT_OPTIONS),
new Dependency(internalDependency, DEFAULT_OPTIONS),
getPublicDependency(internalDependency, DEFAULT_OPTIONS),
getPublicDependency(internalDependency, DEFAULT_OPTIONS),
);
});
});
14 changes: 7 additions & 7 deletions packages/core/core/test/SymbolPropagation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ function assertUsedSymbols(
if (isLibrary) {
let entryDep = nullthrows(
[...graph.nodes.values()].find(
n => n.type === 'dependency' && n.value.sourceAssetId == null,
n => n?.type === 'dependency' && n.value.sourceAssetId == null,
),
);
invariant(entryDep.type === 'dependency');
Expand Down Expand Up @@ -240,12 +240,12 @@ function assertUsedSymbols(
}
}

for (let [nodeId, node] of graph.nodes) {
if (node.type === 'asset') {
for (let [nodeId, node] of graph.nodes.entries()) {
if (node?.type === 'asset') {
let filePath = fromProjectPathUnix(node.value.filePath);
let expected = new Set(nullthrows(expectedAsset.get(filePath)));
assertSetEqual(node.usedSymbols, expected, filePath);
} else if (node.type === 'dependency' && node.value.sourcePath != null) {
} else if (node?.type === 'dependency' && node.value.sourcePath != null) {
let resolutionId = graph.getNodeIdsConnectedFrom(nodeId)[0];
let resolution = nullthrows(graph.getNode(resolutionId));
invariant(resolution.type === 'asset_group');
Expand Down Expand Up @@ -373,14 +373,14 @@ function changeDependency(
): Iterable<[ContentKey, Asset]> {
let sourceAssetNode = nullthrowsAssetNode(
[...graph.nodes.values()].find(
n => n.type === 'asset' && n.value.filePath === from,
n => n?.type === 'asset' && n.value.filePath === from,
),
);
sourceAssetNode.usedSymbolsDownDirty = true;
let depNode = nullthrowsDependencyNode(
[...graph.nodes.values()].find(
n =>
n.type === 'dependency' &&
n?.type === 'dependency' &&
n.value.sourcePath === from &&
n.value.specifier === to,
),
Expand All @@ -396,7 +396,7 @@ function changeAsset(
): Iterable<[ContentKey, Asset]> {
let node = nullthrowsAssetNode(
[...graph.nodes.values()].find(
n => n.type === 'asset' && n.value.filePath === asset,
n => n?.type === 'asset' && n.value.filePath === asset,
),
);
node.usedSymbolsUpDirty = true;
Expand Down
1 change: 1 addition & 0 deletions packages/core/graph/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"node": ">= 12.0.0"
},
"dependencies": {
"@parcel/utils": "^2.9.3",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We once had a PR that removed this dependency to get the graph package working in more environments: #8630

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make the BitSet utils into a separate package?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I had not considered that others would be using our graph package. I guess we would have to release this as a major then (changing the type of nodes)... 😕

I'd really like to avoid yet another package. We were just talking about consolidating them haha. Any other ideas?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I moved BitSet into the graph package, and made the default bundler use it from there instead of utils.

"nullthrows": "^1.1.1"
}
}
Loading
Loading