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

Optimise the Inline Requires plugin #9664

Merged
merged 4 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 26 additions & 16 deletions packages/optimizers/inline-requires/src/InlineRequires.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,26 @@
import {Optimizer} from '@parcel/plugin';
import {parse, print} from '@swc/core';
import {RequireInliningVisitor} from './RequireInliningVisitor';
import type {SideEffectsMap} from './types';
import nullthrows from 'nullthrows';
import SourceMap from '@parcel/source-map';

let publicIdToAssetSideEffects = null;
let assetPublicIdsWithSideEffects = null;

type BundleConfig = {|
publicIdToAssetSideEffects: Map<string, SideEffectsMap>,
assetPublicIdsWithSideEffects: Set<string>,
|};

// $FlowFixMe not sure how to anotate the export here to make it work...
module.exports = new Optimizer<empty, BundleConfig>({
loadBundleConfig({bundle, bundleGraph, tracer}): BundleConfig {
if (publicIdToAssetSideEffects !== null) {
return {publicIdToAssetSideEffects};
if (assetPublicIdsWithSideEffects !== null) {
return {assetPublicIdsWithSideEffects};
}

publicIdToAssetSideEffects = new Map<string, SideEffectsMap>();
assetPublicIdsWithSideEffects = new Set<string>();

if (!bundle.env.shouldOptimize) {
return {publicIdToAssetSideEffects};
return {assetPublicIdsWithSideEffects};
}

const measurement = tracer.createMeasurement(
Expand All @@ -32,19 +31,16 @@ module.exports = new Optimizer<empty, BundleConfig>({
);

bundleGraph.traverse(node => {
if (node.type === 'asset') {
if (node.type === 'asset' && node.value.sideEffects) {
const publicId = bundleGraph.getAssetPublicId(node.value);
let sideEffectsMap = nullthrows(publicIdToAssetSideEffects);
sideEffectsMap.set(publicId, {
sideEffects: node.value.sideEffects,
filePath: node.value.filePath,
});
let sideEffectsMap = nullthrows(assetPublicIdsWithSideEffects);
sideEffectsMap.add(publicId);
}
});

measurement && measurement.end();

return {publicIdToAssetSideEffects};
return {assetPublicIdsWithSideEffects};
},

async optimize({
Expand All @@ -61,7 +57,7 @@ module.exports = new Optimizer<empty, BundleConfig>({
}

try {
const measurement = tracer.createMeasurement(
let measurement = tracer.createMeasurement(
'@parcel/optimizer-inline-requires',
'parse',
bundle.name,
Expand All @@ -72,12 +68,26 @@ module.exports = new Optimizer<empty, BundleConfig>({
const visitor = new RequireInliningVisitor({
bundle,
logger,
publicIdToAssetSideEffects: bundleConfig.publicIdToAssetSideEffects,
assetPublicIdsWithSideEffects:
bundleConfig.assetPublicIdsWithSideEffects,
});

measurement = tracer.createMeasurement(
'@parcel/optimizer-inline-requires',
'visit',
bundle.name,
);
visitor.visitProgram(ast);
measurement && measurement.end();

if (visitor.dirty) {
const measurement = tracer.createMeasurement(
'@parcel/optimizer-inline-requires',
'print',
bundle.name,
);
const result = await print(ast, {sourceMaps: !!bundle.env.sourceMap});
measurement && measurement.end();

let sourceMap = null;
let resultMap = result.map;
Expand Down
59 changes: 18 additions & 41 deletions packages/optimizers/inline-requires/src/RequireInliningVisitor.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@ import type {
Identifier,
} from '@swc/core';
import {Visitor} from '@swc/core/Visitor';
import type {SideEffectsMap} from './types';
import nullthrows from 'nullthrows';

type VisitorOpts = {|
bundle: NamedBundle,
logger: PluginLogger,
publicIdToAssetSideEffects: Map<string, SideEffectsMap>,
assetPublicIdsWithSideEffects: Set<string>,
|};

export class RequireInliningVisitor extends Visitor {
Expand All @@ -26,17 +25,17 @@ export class RequireInliningVisitor extends Visitor {
dirty: boolean;
logger: PluginLogger;
bundle: NamedBundle;
publicIdToAssetSideEffects: Map<string, SideEffectsMap>;
assetPublicIdsWithSideEffects: Set<string>;

constructor({bundle, logger, publicIdToAssetSideEffects}: VisitorOpts) {
constructor({bundle, logger, assetPublicIdsWithSideEffects}: VisitorOpts) {
super();
this.currentModuleNode = null;
this.moduleVariables = new Set();
this.moduleVariableMap = new Map();
this.dirty = false;
this.logger = logger;
this.bundle = bundle;
this.publicIdToAssetSideEffects = publicIdToAssetSideEffects;
this.assetPublicIdsWithSideEffects = assetPublicIdsWithSideEffects;
}

visitFunctionExpression(n: FunctionExpression): FunctionExpression {
Expand All @@ -48,19 +47,14 @@ export class RequireInliningVisitor extends Visitor {
// and also reset the module variable tracking data structures.
//
// (TODO: Support arrow functions if we modify the runtime to output arrow functions)
// For ease of comparison, map the arg identifiers to an array of strings.. this will skip any
// functions with non-identifier args (e.g. spreads etc..)
const args = n.params.map(param => {
if (param.pat.type === 'Identifier') {
return param.pat.value;
}
return null;
});

if (
args[0] === 'require' &&
args[1] === 'module' &&
args[2] === 'exports'
n.params.length === 3 &&
n.params[0].pat.type === 'Identifier' &&
n.params[0].pat.value === 'require' &&
n.params[1].pat.type === 'Identifier' &&
n.params[1].pat.value === 'module' &&
n.params[2].pat.type === 'Identifier' &&
n.params[2].pat.value === 'exports'
) {
// `inModuleDefinition` is either null, or the module definition node
this.currentModuleNode = n;
Expand All @@ -82,7 +76,7 @@ export class RequireInliningVisitor extends Visitor {
// We're looking for variable declarations that look like this:
//
// `var $acw62 = require("acw62");`
let unusedDeclIndexes = [];
let needsReplacement = false;
for (let i = 0; i < n.declarations.length; i++) {
let decl = n.declarations[i];
const init = decl.init;
Expand Down Expand Up @@ -111,41 +105,24 @@ export class RequireInliningVisitor extends Visitor {
//
// This won't work in dev mode, because the id used to require the asset isn't the public id
if (
!this.publicIdToAssetSideEffects ||
!this.publicIdToAssetSideEffects.has(assetPublicId)
this.assetPublicIdsWithSideEffects &&
this.assetPublicIdsWithSideEffects.has(assetPublicId)
) {
this.logger.warn({
message: `${this.bundle.name}: Unable to resolve ${assetPublicId} to an asset! Assuming sideEffects are present.`,
});
} else {
const asset = nullthrows(
this.publicIdToAssetSideEffects.get(assetPublicId),
);
if (asset.sideEffects) {
this.logger.verbose({
message: `Skipping optimisation of ${assetPublicId} (${asset.filePath}) as it declares sideEffects`,
});
// eslint-disable-next-line no-continue
continue;
}
continue;
}

// The moduleVariableMap contains a mapping from (e.g. $acw62 -> the AST node `require("acw62")`)
this.moduleVariableMap.set(variable, init);
// The moduleVariables set is just the used set of modules (e.g. `$acw62`)
this.moduleVariables.add(variable);

this.logger.verbose({
message: `${this.bundle.name}: Found require of ${variable} for replacement`,
});

// Replace this with a null declarator, we'll use the `init` where it's declared.
//
// This mutates `var $acw62 = require("acw62")` -> `var $acw62 = null`
//
// The variable will be unused and removed by optimisation
decl.init = undefined;
unusedDeclIndexes.push(i);
needsReplacement = true;
} else if (
decl.id.type === 'Identifier' &&
typeof decl.id.value === 'string' &&
Expand Down Expand Up @@ -183,10 +160,10 @@ export class RequireInliningVisitor extends Visitor {
this.moduleVariables.add(variable);

decl.init = undefined;
unusedDeclIndexes.push(i);
needsReplacement = true;
}
}
if (unusedDeclIndexes.length === 0) {
if (!needsReplacement) {
return super.visitVariableDeclaration(n);
} else {
this.dirty = true;
Expand Down
2 changes: 0 additions & 2 deletions packages/optimizers/inline-requires/src/types.js

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,15 @@ import {RequireInliningVisitor} from '../src/RequireInliningVisitor';
import assert from 'assert';
import logger from '@parcel/logger';

async function testRequireInliningVisitor(src, sideEffectsMap) {
async function testRequireInliningVisitor(src, sideEffects) {
const ast = await parse(src, {});
const publicIdToAssetSideEffects = new Map();
for (const [id, hasSideEffects] of Object.entries(sideEffectsMap)) {
publicIdToAssetSideEffects.set(id, {sideEffects: hasSideEffects});
}
const assetPublicIdsWithSideEffects = new Set(sideEffects);

const visitor = new RequireInliningVisitor({
bundle: {
name: 'test-bundle',
},
publicIdToAssetSideEffects,
assetPublicIdsWithSideEffects,
logger,
});
visitor.visitProgram(ast);
Expand Down Expand Up @@ -46,9 +43,7 @@ describe('InliningVisitor', () => {
const src = getModule(`
var $abc123 = require('abc123');
console.log($abc123);`);
const result = await testRequireInliningVisitor(src, {
abc123: false,
});
const result = await testRequireInliningVisitor(src, []);
assertEqualCode(
result,
getModule(`var $abc123;
Expand All @@ -67,9 +62,7 @@ describe('InliningVisitor', () => {
var $abc123Default;
console.log((0, parcelHelpers.interopDefault(require('abc123'))).foo());`,
);
const result = await testRequireInliningVisitor(src, {
abc123: false,
});
const result = await testRequireInliningVisitor(src, []);
assertEqualCode(result, expected);
});

Expand All @@ -82,10 +75,7 @@ describe('InliningVisitor', () => {
var $abc456;
console.log($abc123);
console.log((0, require('abc456')));`);
const result = await testRequireInliningVisitor(src, {
abc123: true,
abc456: false,
});
const result = await testRequireInliningVisitor(src, ['abc123']);
assertEqualCode(result, expected);
});
});
Loading