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

Emit /*@__PURE__*/ before class declaration #452

Closed
IgorMinar opened this issue Mar 27, 2017 · 24 comments
Closed

Emit /*@__PURE__*/ before class declaration #452

IgorMinar opened this issue Mar 27, 2017 · 24 comments

Comments

@IgorMinar
Copy link
Contributor

Based on this research, I believe that if we were able to emit a newly supported /*@__PURE__*/ annotation in front of class declaration and ensure that it gets preserved in the final down-leveled output, @angular/cli and any other Uglify-based build tool chains would be able to use dead-code elimination tricks to remove all unused classes (pipes, directives and components) with the exception of unused services.

There is a feature request microsoft/TypeScript#13721 to add this functionality to tsc, but it's unclear when/if it's going to be implemented, so I wonder how easy it would be to implement this kind of feature in tsickle so that we can experiment.

What I'd like is given the following input:

export class SomeClass {}

I'd like tsc + tscikle to output downleveled es5 code that looks like this:

var SomeClass = /*@__PURE__*/(function () {
  function SomeClass() {}
  return SomeClass;
}());

If for some reason we have a concern that /*@__PURE__*/ could cause headaches for Closure, we can also use /*#__PURE__*/, which is equally supported by Uglify.

More info:

@mprobst
Copy link
Contributor

mprobst commented Mar 27, 2017 via email

@alexeagle
Copy link
Contributor

Answering Igor's question from another thread: technically speaking, this would be very easy to do.

@IgorMinar
Copy link
Contributor Author

@mprobst webpack doesn't eliminate anything that is reachable from the entry point via ES imports. It relies on Uglify to remove the module bodies and minify the code. Uglify however can't safely determine if a class downleveled to an IIFE is safe to remove, so it retains them all.

@alexeagle I'm at the point in my research where this issue is overshadowing all the other issues and I can't make progress until this issue is fixed. Would you or someone else from the tooling team be able to implement this on a branch so that I can't test it and verify that the impact is as big as it seems?

@evmar
Copy link
Contributor

evmar commented Mar 27, 2017

I imagine you can use a sed script to get most of this for experimentation purposes.

Unfortunately it's a bit hard to hack this into tsickle. We either see the code before downlevelling (in which case it's a "class" keyword and there's no place to put the comment) or after downlevelling (in which case it's a soup of function calls and we don't know which ones are classes anymore). Hooking into the TS transformation pipeline would probably allow this kind of change but that's a new API that we don't use yet.

@IgorMinar
Copy link
Contributor Author

@evmar this is exactly what I was afraid of :-(

@IgorMinar
Copy link
Contributor Author

@evmar sed is kind of impractical, but I can try it. I wonder if forking typescript and experimenting with a fork would be better/faster way to get some reliable results that would allow me to estimate the impact of this change.

@evmar
Copy link
Contributor

evmar commented Mar 27, 2017

TS is now a series of transformations. Here's the place where "class" is transformed into the function:
https://github.com/Microsoft/TypeScript/blob/c949116fa92a0f7d4683f8f698be47bf1b15eb92/src/compiler/transformers/es2015.ts#L661

However, I don't know how to adjust that to insert a comment. I think @rkirov has played with this API and would know.

@IgorMinar
Copy link
Contributor Author

This would however mean that we'd need our toolchain updated to TypeScript HEAD/2.3, which is not an easy feat.

@evmar
Copy link
Contributor

evmar commented Mar 27, 2017

Yeah. I think for experimentation purposes you can check out an old version of TS and hack in the change locally. I'm not sure if that's easier than doing something with sed.

I think the fastest thing you can do is something like

grep -r '^class ' * > list_of_classes.py

followed by hacking up that output into a that finds those specific "var ..." lines and inserts your annotation.

@alexeagle
Copy link
Contributor

Even with TS 2.3 you can only add transforms at the beginning or the end, so it's the same power tsickle has now (just a more composable and performant implementation)
I'm working on that upgrade now because I need it for TypeScript in Bazel. (But we won't land it until TS 2.3 is released).

I see you're saying that
https://www.typescriptlang.org/play/#src=%2F*%40__PURE__*%2F%0D%0Aclass%20Foo%20%7B%7D%0D%0A
puts the comment in the wrong place, it's before the var but for rollup it needs to be on the RHS of the assignment? I imagine tsickle could re-write the comment to move it after TS downlevels.

@IgorMinar
Copy link
Contributor Author

IgorMinar commented Mar 28, 2017

to my big surprise I found the following JS regexp replacement working quite well:

function(fileBody) {
    return fileBody
      .replace(
          /^(var (\S+) = )(\(function \(\) \{\n(?:    (?:\/\*\*| \*|\*\/|\/\/)[^\n]*\n)*    function \2\([^\)]*\) \{\n)/mg,
          '$1/*@__PURE__*/$3'
      )
      .replace(
          /^(var (\S+) = )(\(function \(_super\) \{\n    __extends\(\2, _super\);\n)/mg,
          '$1/*@__PURE__*/$3'
      );
};

@evmar
Copy link
Contributor

evmar commented Mar 28, 2017

I like it!

@IgorMinar
Copy link
Contributor Author

I dumped my hacky tool on github as a gist in case anyone wants to play with this before it's properly implemented either in typescript or in tsickle: https://gist.github.com/IgorMinar/09eadcb221a8253162078210639a344d

@IgorMinar
Copy link
Contributor Author

Oh, and btw the size gains are totally worth the effort of rolling this out properly.

@kzc
Copy link

kzc commented Mar 30, 2017

For best /*@__PURE__*/ IIFE annotation results use the CLI command:

uglifyjs --compress toplevel=true,passes=3

or the minify() options:

compress: {
    toplevel: true,
    passes: 3,
}

See: mishoo/UglifyJS#1261 (comment)

@rkirov
Copy link
Contributor

rkirov commented Mar 30, 2017

AFAIK, there is not way to hook into the pipeline of transforms TS compiler does. The only options are run before all of them, or run after all of them.

(side rant): I really wish the name picked was not 'PURE' and something more like 'UglifyHint' or 'ClassExpressionHint' etc. Purity and side-effects are concepts that everyone feels like they know, yet when hard-pressed to define them explicitly, turns out everyone has slightly different definition. People have spent years of research in trying to reshape their whole languages just to make purity and side-effects explicit (see Haskell, IO monad, etc). It is not clear how successful they are, but it is clearly not an easy task.

I am forseeing 'Sticky_Pure', "pure" but pls keep it around :)

@evmar
Copy link
Contributor

evmar commented Mar 30, 2017

If we really want this we could do a thing where we emit classes as

/** @PURE */ class Foo

and then let TS emit, and then in a second postprocessing step search for the @pure annotations and move them to the right place.

Would be kind of a pain to implement though. :~(
I wonder if we can modify uglify to handle @pure before the variable.

@evmar
Copy link
Contributor

evmar commented Mar 30, 2017

Re the semantics of "pure", obligatory link:
https://mail.mozilla.org/pipermail/rust-dev/2013-April/003926.html

@mprobst
Copy link
Contributor

mprobst commented Mar 30, 2017 via email

@IgorMinar
Copy link
Contributor Author

IgorMinar commented Mar 30, 2017 via email

@kzc
Copy link

kzc commented Mar 30, 2017

I wonder if we can modify uglify to handle @pure before the variable.

The annotation was intentionally designed to be before the specific function call because it is unambiguous and the comment survives numerous uglify transforms including var combining.

As for the name of the annotation, it's not important as it'd be in intermediate ES5 generated code never seen by coders. Also, the person who built the bike shed gets to name it.

We are talking double digits. 60-70% in some cases.

Good stuff. Seems to be an easy win for your project.

@IgorMinar
Copy link
Contributor Author

@kzc I tried using

    toplevel: true,
    passes: 3,
}

in my webpack config, but it doesn't seem to have any effect.

Why do you believe that it was supposed to make a difference?

@kzc
Copy link

kzc commented Apr 3, 2017

@IgorMinar See example in: mishoo/UglifyJS#1261 (comment)

Remove the flags and see what happens.

@IgorMinar
Copy link
Contributor Author

I'm going to close this issue, because if we were to implement this in tsickle, we'd be missing out on annotating anything that comes into the app from node_modules that has not been processed by tsickle. Instead we are either going to rely on tsc to do this (microsoft/TypeScript#13721 ), or in the short term we'll have our own tool integrated into the angular-cli that would prefix all the ts and js code that makes up an application be annotated in this way.

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

No branches or pull requests

6 participants