refactor: changed __UglifyJsPlugin dereference to use WeakSet instead

[hulkish]

So, as a proof of concept... I created this: https://github.com/hulkish/dummy

Pull that repo, then run:

yarn
node scripts/profile.js

You will see an output which looks like:

[email protected] compile...
[email protected] files length 68
[email protected] profiler.startProfiling()
[email protected]: 27549.784ms
[email protected] profiler.stopProfiling()
[email protected] profile.export()
[email protected] profile saved: /Users/shargrove/dev/repos/dummy/[email protected]
[email protected] compile...
[email protected] files length 68
[email protected] profiler.startProfiling()
[email protected]: 14367.834ms
[email protected] profiler.stopProfiling()
[email protected] profile.export()
[email protected] profile saved: /Users/shargrove/dev/repos/dummy/[email protected]

then, you can compare the output of these 2 folders:

• dist/[email protected]
• dist/[email protected]

The output is the same, but look at the significant difference in time.

[michael-ciniawsky]

Change the base branch back to master please, since it seem to landed there yet :)

[Jessidhia]

Interesting, I didn't expect using a WeakSet to actually improve performance by much; it just "felt right" considering how the extra key was added and used. But I guess the cost of forcing a hidden class change by adding an extra key was high enough.

It looks like this needs a rebase to get rid of the indent changes, though.

[mikesherov]

@Kovensky that is to say, this was the old code:

asset = compilation.assets[file]
if(asset.__UglifyJsPlugin) {
	compilation.assets[file] = asset.__UglifyJsPlugin;
	return;
}
outputSource = uglify();
compilation.assets[file] = outputSource;
asset.__UglifyJsPlugin = outputSource;

So there's a dangling reference there. No object inside compilation.assets ever gets the prop appended! e.g. https://jsfiddle.net/jqs4c96k/2/

[mikesherov]

Oh interesting, the weakSet is in the wrong spot? Can different compilations have the same asset?

[hulkish]

@mikesherov what u mean wrong spot?

[mikesherov]

Can be placed at RequestShortener's scope?

[hulkish]

You're right... I considered that but i wonder if in principle it's actually better to keep it in 'optimize-chunk-assets` handler scope... For ex... i thought about how this might behave in watch mode and wondered if this would cause an issue.

[mikesherov]

It can't possibly, it's a WeakSet! It won't leak memory, and a match will only happen when it truly should happen!

[hulkish]

Ah, very good point.

[michael-ciniawsky]

Sry for the heads up, but could we triage landing/not landing uglify-es (#55) first before optimizing further 🙃 ?

[mikesherov]

YESSSSS

[michael-ciniawsky]

ALRIGHTTTT 😛 :D