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

Basic support of asm.js uglification #373

Closed
wants to merge 2 commits into from

Conversation

vibornoff
Copy link

A had a problem when trying to uglify asm.js modules in my asmcrypto.js project.

To fix that issue I added basic support:

  • Asm.js is turned on by directive "use asm", so when it is met,
  • Name mangler doesn't reuse names in the inner scopes of the modules, and
  • Compression is turned off for that modules.


// asm.js: don't mask module-scope names
var asm = this.has_directive("use asm"),
pasm = this.parent_scope ? this.parent_scope.has_directive("use asm") : false;
Copy link
Owner

Choose a reason for hiding this comment

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

Note that has_directive returns the scope where the directive is found (so it looks into the parent scope too), therefore if the directive is specified on the parent then: this.has_directive("use asm") == this.parent_scope.has_directive("use asm");

Copy link
Author

Choose a reason for hiding this comment

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

Take a closer look: pasm is set to true only if the current scope is nested into the asm scope.
So it's false for top-level asm scope even it has "use asm" directive.

Copy link
Owner

Choose a reason for hiding this comment

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

OK. It's just that it's unclear to me what this whole patch does, can you explain in plain English?

My understanding is that you need to not shadow the name of the function where "use asm" is found, is that all?

@vibornoff
Copy link
Author

what this whole patch does, can you explain in plain English?

Sorry, my fault.

Asm.js — is a highly optimizable and restricted subset of JavaScript. For more information take a look into Wikipedia article and latest Asm.js specification. It's in heavy development now and available only in Firefox (>= 22).

This patch attempts to add basic support of asm.js handling into your project as UglifyJS2 doesn't know anything about asm.js and when I feed valid asm.js code it produces invalid uglified output (though output remains valid JS code).

My understanding is that you need to not shadow the name of the function where "use asm" is found, is that all?

There is a number of issues:

  • internal scopes of asm.js module can't overload names existing in the top-level asm.js scope
  • (seems bug of asm.js implementation) name of the module can't be reused inside although it is out of the module scope
  • any optimization have to be bypassed exept maybe drop_unused since asm.js has different typing paradigm (see spec)

Also asm.js have its own validation rules but they are out of the scope of this project since it is for general-purpose JavaScript. My patch is focused on making UglifyJS2 not to produce invalid asm.js code when valid is supplied.

@vibornoff vibornoff closed this Dec 17, 2013
@vibornoff vibornoff reopened this Dec 17, 2013
@vibornoff
Copy link
Author

Rebased

@vibornoff
Copy link
Author

Up

@pohutukawa
Copy link

It would be really nice if asm.js support was provided out of the box by UglifyJS2. It can be a real stopper for automatic deployment of JS modules via npm using the dependencies in package.json.

@bored-engineer
Copy link

Bump

@avdg
Copy link
Contributor

avdg commented Jun 28, 2014

I am wondering why this ticket is still being open. However given the comments in the patches, I guess we need a separate maintainer for the asm module with someone who knows asm.js better (just take this as a proposal, other ideas are welcome).

I am not sure if that's doable and/or its something that @mishoo wants.

Comments?

@diegocr
Copy link

diegocr commented Sep 30, 2014

+1 -- Bump

@vibornoff
Copy link
Author

Just rebased my old commits with few minor modifications

@rvanvelzen
Copy link
Collaborator

This is probably outdated, so I'm closing this PR. Feel free to re-submit if you think it should be merged.

@rvanvelzen rvanvelzen closed this Jun 9, 2016
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.

7 participants