Skip to content
This repository has been archived by the owner on Dec 5, 2019. It is now read-only.

Commit

Permalink
perf(sourcemaps): reduce memory usage (#276)
Browse files Browse the repository at this point in the history
  • Loading branch information
evilebottnawi authored and joshwiens committed Apr 18, 2018
1 parent e94a401 commit 0b11134
Showing 1 changed file with 27 additions and 16 deletions.
43 changes: 27 additions & 16 deletions src/index.js
Expand Up @@ -48,11 +48,18 @@ class UglifyJsPlugin {
...uglifyOptions,
},
};
this.sourceMapsCache = new WeakMap();
}

static buildError(err, file, sourceMap, requestShortener) {
buildError(err, file, inputSourceMap, requestShortener) {
// Handling error which should have line, col, filename and message
if (err.line) {
const sourceMapCacheKey = { file };

This comment has been minimized.

Copy link
@lojjic

lojjic May 4, 2018

While trying to track down why the 1.2.5 release made our build 4x longer, we came across this change which appears to be the culprit.

This doesn't seem to be working as it was intended, since it creates a new sourceMapCacheKey object every time which results in a cache miss on every call. If I change this to a normal Map keyed by the file string, our build time goes back to normal.

I'm happy to submit a PR, but since this is unfamiliar territory I'd like some guidance/confirmation first: is it important that this remain a WeakMap, which would require a way to map file to unique key objects, or is switching to a Map keyed by file string sufficient? Thanks in advance.

This comment has been minimized.

Copy link
@alexander-akait

alexander-akait May 4, 2018

Member

@lojjic Good question. after create error we don't have reference on source map and garbage collector
remove object from memory and it is reduce memory usage. but if you have many errors/warnings you increase build time, because each time we should create new source map. I would recommend creating a new problem for this and continuing the discussion there.

This comment has been minimized.

Copy link
@lojjic

lojjic May 4, 2018

Thanks, opened #286

let sourceMap = this.sourceMapsCache.get(sourceMapCacheKey);
if (!sourceMap) {
sourceMap = new SourceMapConsumer(inputSourceMap);
this.sourceMapsCache.set(sourceMapCacheKey, sourceMap);
}
const original = sourceMap && sourceMap.originalPositionFor({
line: err.line,
column: err.col,
Expand All @@ -67,11 +74,20 @@ class UglifyJsPlugin {
return new Error(`${file} from UglifyJs\n${err.message}`);
}

static buildWarning(warning, file, sourceMap, warningsFilter, requestShortener) {
if (!file || !sourceMap) {
buildWarning(warning, file, inputSourceMap, warningsFilter, requestShortener) {
if (!file || !inputSourceMap) {
return warning;
}

const sourceMapCacheKey = { file };

let sourceMap = this.sourceMapsCache.get(sourceMapCacheKey);

if (!sourceMap) {
sourceMap = new SourceMapConsumer(inputSourceMap);
this.sourceMapsCache.set(sourceMapCacheKey, sourceMap);
}

const match = warningRegex.exec(warning);
const line = +match[1];
const column = +match[2];
Expand Down Expand Up @@ -114,15 +130,14 @@ class UglifyJsPlugin {
.concat(compilation.additionalChunkAssets || [])
.filter(ModuleFilenameHelpers.matchObject.bind(null, this.options))
.forEach((file) => {
let sourceMap;
let inputSourceMap;
const asset = compilation.assets[file];
if (uglifiedAssets.has(asset)) {
return;
}

try {
let input;
let inputSourceMap;

if (this.options.sourceMap && asset.sourceAndMap) {
const { source, map } = asset.sourceAndMap();
Expand All @@ -131,11 +146,8 @@ class UglifyJsPlugin {

if (utils.isSourceMap(map)) {
inputSourceMap = map;
sourceMap = new SourceMapConsumer(inputSourceMap);
} else {
inputSourceMap = map;
sourceMap = null;

compilation.warnings.push(
new Error(`${file} contain invalid source map`),
);
Expand All @@ -157,7 +169,6 @@ class UglifyJsPlugin {
const task = {
file,
input,
sourceMap,
inputSourceMap,
commentsFile,
extractComments: this.options.extractComments,
Expand All @@ -177,10 +188,10 @@ class UglifyJsPlugin {
tasks.push(task);
} catch (error) {
compilation.errors.push(
UglifyJsPlugin.buildError(
this.buildError(
error,
file,
sourceMap,
inputSourceMap,
requestShortener,
),
);
Expand All @@ -194,17 +205,17 @@ class UglifyJsPlugin {
}

results.forEach((data, index) => {
const { file, input, sourceMap, inputSourceMap, commentsFile } = tasks[index];
const { file, input, inputSourceMap, commentsFile } = tasks[index];
const { error, map, code, warnings, extractedComments } = data;

// Handling results
// Error case: add errors, and go to next file
if (error) {
compilation.errors.push(
UglifyJsPlugin.buildError(
this.buildError(
error,
file,
sourceMap,
inputSourceMap,
requestShortener,
),
);
Expand Down Expand Up @@ -264,10 +275,10 @@ class UglifyJsPlugin {
// Handling warnings
if (warnings && warnings.length > 0) {
warnings.forEach((warning) => {
const builtWarning = UglifyJsPlugin.buildWarning(
const builtWarning = this.buildWarning(
warning,
file,
sourceMap,
inputSourceMap,
this.options.warningsFilter,
requestShortener,
);
Expand Down

0 comments on commit 0b11134

Please sign in to comment.