Skip to content

Rewrite MinifyLocals from Uglify to Acorn. NFC #13621

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

Merged
merged 15 commits into from
Mar 8, 2021
Merged

Rewrite MinifyLocals from Uglify to Acorn. NFC #13621

merged 15 commits into from
Mar 8, 2021

Conversation

kripken
Copy link
Member

@kripken kripken commented Mar 8, 2021

Basically a simple rewrite from one AST to another, but also using slightly
more modern JS while doing so (Set, Map). Some parts differ though as
the ASTs are not compatible, so the algorithm is not 100% identical, but the
results should be.

Test changes are just minor whitespace, caused by the different whitespace
style that Uglify used to emit vs the new one.

Verified on wasm2js2, 3, s, z which we do not run on CI here.

After this we have just one remaining pass using Uglify, MinifyGlobals, after
which we can remove it entirely.

cc @walkingeyerobot

@kripken kripken requested a review from sbc100 March 8, 2021 17:03
@sbc100
Copy link
Collaborator

sbc100 commented Mar 8, 2021

Is this part of #12410?

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Great!

I only briefly glanced over the actual algorithm, assuming that its its a roughly 1-to-1 with the existing code.

@@ -1444,9 +1606,11 @@ var registry = {
applyDCEGraphRemovals: applyDCEGraphRemovals,
minifyWhitespace: function() { minifyWhitespace = true },
noPrint: function() { noPrint = true },
last: function() {},
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this?

Copy link
Member Author

Choose a reason for hiding this comment

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

The python code uses "last" as a marker for knowing that nothing will run next. The acorn code doesn't actually need it, as you can see here, but adding this lets the existing python driver code work unmodified. Basically this makes the new pass implement the exact same API surface as the old pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

(A refactoring on the python side could remove it, i'll add a TODO)

var minifiedNames = [];
var minifiedState = [0];

function ensureMinifiedNames(n) { // make sure the nth index in minifiedNames exists. done 100% deterministically
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you put the comment on its own line (before the function I guess?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@kripken
Copy link
Member Author

kripken commented Mar 8, 2021

Is this part of #12410?

It could be perhaps as part of a larger refactoring. But the Acorn code uses Terser for its output, so removing Uglify would not affect our usage of Terser.

@kripken kripken merged commit aa66f92 into master Mar 8, 2021
@kripken kripken deleted the minloc branch March 8, 2021 18:03
@walkingeyerobot
Copy link
Collaborator

Awesome, thanks for the heads up!

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

Successfully merging this pull request may close these issues.

3 participants