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

Add support for annotations improving minification and rewriting #178

Open
Ginden opened this issue Oct 5, 2016 · 9 comments
Open

Add support for annotations improving minification and rewriting #178

Ginden opened this issue Oct 5, 2016 · 9 comments

Comments

@Ginden
Copy link

Ginden commented Oct 5, 2016

Eg. ability to declare "pure" functions:

class Bar {
   /* babili: no-side-effects */
   constructor() {
       someFunction(this);
  }
}
new Bar();

Would result in removing new Bar() because of no-side-effects annotation.
Other possible annotations include: dont-modify-argument, constant-expression, always-new (function always return unique object - like foo = ()=>({}) - all calls to foo() results in new objects) or similar.

@boopathi
Copy link
Member

boopathi commented Oct 5, 2016

babili doesn't support any annotations. Even if it does in future, IMO, it should be in the direction of either jsdoc style annotation or support for Flow. I feel re-inventing something specific to babili in user-code is a bad idea.

@hzoo
Copy link
Member

hzoo commented Oct 5, 2016

Yep I would agree - we can certainly read in some type annotations or something else.

@kangax
Copy link
Member

kangax commented Oct 5, 2016

Yes, we're definitely planning to integrate Flow for better DCE. We could revisit annotations in the future for additional savings.

@hzoo
Copy link
Member

hzoo commented Oct 5, 2016

If we do that flow-strip-types will just have to be run last

@Ginden
Copy link
Author

Ginden commented Oct 6, 2016

I feel re-inventing something specific to babili in user-code is a bad idea.

Actually, eslint use this syntax - /* eslint: some-param */, so it wouldn't be something very specific to one tool.

@kangax
Copy link
Member

kangax commented Oct 24, 2016

I came across few patterns in our code which could benefit from these kind of annotations.

Our DCE currently does inlining of nearby statements like so:

var x = { foo: 'bar' };
module.exports = x;

becomes:

module.exports = { foo: 'bar' };

Problem is, we can only do this when the expression we're inlining is "pure", meaning that it has no side effects:

var x = foo();
module.exports = x;

...will not be inlined since we don't know what foo() does and how it affects the rest of the code/state. In other words, inlining expressions changes order of evaluation.

Even if we can inline it in this case, since we know that no other actions can occur here, there's other cases where it's definitely not safe, and so we would need a smarter analysis, pretty much opening up a can of worms:

Simple example:

var x = foo();
module.exports = bar() + x;

changes order of execution (of foo and bar) if inlined:

module.exports = bar() + foo();

Or more complex:

var x = getSmthAndLog(); // logging
module.exports = (function() { 
  // function exits early for whatever reason
  // logging never occurs if `x` was to be replaced-inlined with `getSmthAndLog()`
  if (x) { smth() }
})();

In our FB codebase we have cases like this:

var someModule = require('someModule');
var SomeConstant = someModule({
  foo: '...',
  bar: '...'
});
module.exports = SomeConstant;

which gets minified to:

moduleWrapperAbstraction(..., function(a, b, c, ...) {
    var a = b({
        CREATING: "",
        DELETING: "",
        LOADING: "",
        UPDATING: ""
    });
    c.exports = a;
});

Notice that b() doesn't get inlined due to it possibly having side effect. In this case, we can just be smarter about RHS being a simple identifier and recognizing that it's ok to inline even impure expression.

With more complex cases, I wonder if annotation of some sort could help.

Either annotating expression as pure/having-no-side-effects:

var SomeConstant = someModule({ /* pure-expr: true */
  foo: '...',
  bar: '...'
});

// or directly annotating identifier as something that's safe to inline:

var SomeConstant /* inlineable: true */ = someModule({
  foo: '...',
  bar: '...'
});

Another (likely better) option is to annotate function/module as non-side-effecty during declaration, so that each single assignment and caller wouldn't concern themselves with that. Unfortunately, that's not something we can do right away since this will require global awareness of the entire functions/modules dependency graph with all the consequences/complexities.

Thoughts?

/cc @sebmarkbage @bmaurer

@ForbesLindesay
Copy link

I think long term we need to get more global awareness into our minifier so that we can detect that b is pure and then inline it, without annotations.

@Ginden
Copy link
Author

Ginden commented Oct 26, 2016

Actually it's impossible to reliably detect pure functions in JavaScript, unless they are constant expressions and take only well defined parameters and not use standard library.

Any property access can result in hitting Proxy instance with side-effects. Any global method can be overridden.

In Node.js it's even possible to override require or set setter on module.exports. So everything is based on some kind of assumptions - eg. right now Babili can break code that rely on overriden Object.prototype.toString. Or overriden global constructors like String, Symbol, Array and others.

And proving pureness of function is actually equivalent to halting problem, so it's undecidable in general case.

@ForbesLindesay
Copy link

Any property access can result in ... side-effects

Yes, but you can also use fn.name in your code to identify different functions, but we still minify function names. I think assuming property access is side effect free is a reasonable assumption to make.

In Node.js it's even possible to override require

Again, I think it's a reasonable level of assumption for a minifier to assume this will not be the case.

proving pureness of function is actually equivalent to halting problem

The good news is, we can get this wrong some of the time and still have a useful implementation. For example, I have a module, halting-problem, which identifies many cases where a program never halts. It has proven extremely useful for preventing live editing environments from freezing up.

We can do something similar here, by making a best effort at detecting pure functions, but bailing when we see loops that might not terminate etc.

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

No branches or pull requests

5 participants