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

Hoist RegExp Literals in SourceMapDevToolPlugin #15720

Closed
TheLarkInn opened this issue Apr 26, 2022 · 8 comments · Fixed by #15722
Closed

Hoist RegExp Literals in SourceMapDevToolPlugin #15720

TheLarkInn opened this issue Apr 26, 2022 · 8 comments · Fixed by #15722

Comments

@TheLarkInn
Copy link
Member

Bug report

What is the current behavior?
The use of RegExp literals create unnecessary object references/allocations especially in iterated hot paths like SourceMapDevToolPlugin. We should hoist these to the top of file and give them useful names so people understand what the heck they do.

If the current behavior is a bug, please provide the steps to reproduce.
Some Examples:

Mapping over a RegExp literal:

pattern = contenthash.map(quoteMeta).join("|");

Big Async ForEach Statement with multiple RexExp Literal statements:

asyncLib.each(
tasks,
(task, callback) => {
const assets = Object.create(null);
const assetsInfo = Object.create(null);
const file = task.file;
const chunk = fileToChunk.get(file);
const sourceMap = task.sourceMap;
const source = task.source;
const modules = task.modules;
reportProgress(
0.5 + (0.5 * taskIndex) / tasks.length,
file,
"attach SourceMap"
);
const moduleFilenames = modules.map(m =>
moduleToSourceNameMapping.get(m)
);
sourceMap.sources = moduleFilenames;
if (options.noSources) {
sourceMap.sourcesContent = undefined;
}
sourceMap.sourceRoot = options.sourceRoot || "";
sourceMap.file = file;
const usesContentHash =
sourceMapFilename &&
/\[contenthash(:\w+)?\]/.test(sourceMapFilename);
// If SourceMap and asset uses contenthash, avoid a circular dependency by hiding hash in `file`
if (usesContentHash && task.assetInfo.contenthash) {
const contenthash = task.assetInfo.contenthash;
let pattern;
if (Array.isArray(contenthash)) {
pattern = contenthash.map(quoteMeta).join("|");
} else {
pattern = quoteMeta(contenthash);
}
sourceMap.file = sourceMap.file.replace(
new RegExp(pattern, "g"),
m => "x".repeat(m.length)
);
}
/** @type {string | false} */
let currentSourceMappingURLComment = sourceMappingURLComment;
if (
currentSourceMappingURLComment !== false &&
/\.css($|\?)/i.test(file)
) {
currentSourceMappingURLComment =
currentSourceMappingURLComment.replace(
/^\n\/\/(.*)$/,
"\n/*$1*/"
);
}
const sourceMapString = JSON.stringify(sourceMap);
if (sourceMapFilename) {
let filename = file;
const sourceMapContentHash =
usesContentHash &&
/** @type {string} */ (
createHash(compilation.outputOptions.hashFunction)
.update(sourceMapString)
.digest("hex")
);
const pathParams = {
chunk,
filename: options.fileContext
? relative(
outputFs,
`/${options.fileContext}`,
`/${filename}`
)
: filename,
contentHash: sourceMapContentHash
};
const { path: sourceMapFile, info: sourceMapInfo } =
compilation.getPathWithInfo(
sourceMapFilename,
pathParams
);
const sourceMapUrl = options.publicPath
? options.publicPath + sourceMapFile
: relative(
outputFs,
dirname(outputFs, `/${file}`),
`/${sourceMapFile}`
);
/** @type {Source} */
let asset = new RawSource(source);
if (currentSourceMappingURLComment !== false) {
// Add source map url to compilation asset, if currentSourceMappingURLComment is set
asset = new ConcatSource(
asset,
compilation.getPath(
currentSourceMappingURLComment,
Object.assign({ url: sourceMapUrl }, pathParams)
)
);
}
const assetInfo = {
related: { sourceMap: sourceMapFile }
};
assets[file] = asset;
assetsInfo[file] = assetInfo;
compilation.updateAsset(file, asset, assetInfo);
// Add source map file to compilation assets and chunk files
const sourceMapAsset = new RawSource(sourceMapString);
const sourceMapAssetInfo = {
...sourceMapInfo,
development: true
};
assets[sourceMapFile] = sourceMapAsset;
assetsInfo[sourceMapFile] = sourceMapAssetInfo;
compilation.emitAsset(
sourceMapFile,
sourceMapAsset,
sourceMapAssetInfo
);
if (chunk !== undefined)
chunk.auxiliaryFiles.add(sourceMapFile);
} else {
if (currentSourceMappingURLComment === false) {
throw new Error(
"SourceMapDevToolPlugin: append can't be false when no filename is provided"
);
}
/**
* Add source map as data url to asset
*/
const asset = new ConcatSource(
new RawSource(source),
currentSourceMappingURLComment
.replace(/\[map\]/g, () => sourceMapString)
.replace(
/\[url\]/g,
() =>
`data:application/json;charset=utf-8;base64,${Buffer.from(
sourceMapString,
"utf-8"
).toString("base64")}`
)
);
assets[file] = asset;
assetsInfo[file] = undefined;
compilation.updateAsset(file, asset);
}
task.cacheItem.store({ assets, assetsInfo }, err => {
reportProgress(
0.5 + (0.5 * ++taskIndex) / tasks.length,
task.file,
"attached SourceMap"
);
if (err) {
return callback(err);
}
callback();
});
},
err => {
reportProgress(1.0);
callback(err);
}
);

What is the expected behavior?
Less memory allocation, less garbage collection triggered.

Happy to PR this. There are quite a few of these examples still lying around webpack that should be cleaned up but I'll start with this.

@markjm
Copy link
Contributor

markjm commented Apr 26, 2022

This makes sense to me - I wonder if you have any perf traces or anything that could show any marked improvement from this change?

@TheLarkInn
Copy link
Member Author

This makes sense to me - I wonder if you have any perf traces or anything that could show any marked improvement from this change?

I'll try and find something to show. I won't have before and after, however I'll be able to show the hot path from a memory profiling trace.

@TheLarkInn
Copy link
Member Author

image

@markjm Here is an example of mem allocation hotpath if this helps. This tree is during the forEach calls referenced in the PR.

@webpack-bot
Copy link
Contributor

This issue had no activity for at least three months.

It's subject to automatic issue closing if there is no activity in the next 15 days.

@alexander-akait
Copy link
Member

bump

@webpack-bot
Copy link
Contributor

Issue was closed because of inactivity.

If you think this is still a valid issue, please file a new issue with additional information.

@alexander-akait
Copy link
Member

@TheLarkInn Do we still need it?

@TheLarkInn
Copy link
Member Author

I'll address PR comments and then we can reopen and close once we merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants