Add mark eval scopes helper and deopt DCE fn unused params #371

boopathi · 2017-01-10T18:49:59Z

boopathi · 2017-01-10T19:04:06Z

The helper attaches to the scope object directly instead of returning the list of scopes that contain eval. This is to avoid traversing the program twice - mangler, dce.

boopathi · 2017-01-10T20:01:21Z

Also, this now only deopts eval scopes for only a few transforms under the visitor Scope. Have to look into other places in DCE that require deopt for eval.

kangax · 2017-01-10T23:48:52Z

packages/babel-plugin-minify-dead-code-elimination/__tests__/dead-code-elimination-test.js

+    `);
+
+    const expected = unpad(`
+      function a(b, c, d) {


Will this work with nested functions too? i.e. making sure it doesn't mangle top-level vars either.

added more tests

kangax · 2017-01-10T23:49:56Z

packages/babel-helper-mark-eval-scopes/src/index.js

@@ -0,0 +1,52 @@
+"use strict";
+
+const EVAL_SCOPE_MARKER = "evalInScope";


Should we use Symbol instead?

yes. Will change. I was using scope.evalInScope directly.

kangax

I feel like this is something that should be done on Babel level? So that you can just ask scope.hasDirectEval or something along those lines.

/cc @hzoo

boopathi · 2017-01-11T10:17:12Z

I feel like this is something that should be done on Babel level

I think so too, but if we keep it in babel, it should be CORRECT rather than optimised for a few use cases. Like now, we don't recompute between multiple calls of hasEval()/isUnsafeScope() - as we assume that we don't create new scopes. If we do, we should manually recompute these things by calling markEvalScopes() again.

If we implement this in babel, every call of hasEval/isUnsafeScope should trigger scope.traverse to find if it contains eval. And there are other cases like - eval is removed.

kangax · 2017-01-13T23:12:02Z

Yeah, this brings us again to the usual issues of keeping track of proper state and revalidation. Just like we don't update bindings or certain path information right now after changing nodes.

kangax · 2017-01-17T22:21:56Z

packages/babel-plugin-minify-dead-code-elimination/src/index.js

@@ -710,6 +715,10 @@ module.exports = ({ types: t, traverse }) => {
          traverse.clearCache();
          path.scope.crawl();

+          if (!isEvalScopesMarked(path.scope)) {
+            markEvalScopes(path);


I'd love to merge this for now.

Just one suggestion — let's move isEvalScopesMarked into markEvalScopes (checking it within and returning if scope is already marked). That way API becomes simpler (markEvalScopes(path)), there's less vars to exports, etc.

Another thing to consider is that markEvalScopes operates on path, and hasEval operates on scope. In an ideal world, that means these methods belong to those objects — path.markScopesWithEval, scope.hasDirectEvals.

I didn't put isMarked inside markEvalScopes because if something is changed after marking - we need to remark.

that means these methods belong to those objects — path.markScopesWithEval, scope.hasDirectEvals

Yes!!

Do you want to implement them in Babel instead?

/cc @hzoo what do you think ?

kangax

Please see inline

kangax · 2017-01-19T21:02:22Z

Merging for now. Later, we can implement this in Babel instead.

kangax · 2017-01-19T21:02:44Z

@boopathi thanks for a speedy fix!

boopathi added 2 commits January 10, 2017 19:49

Add mark eval scopes

5aaa71f

Fix readme bug as a result of copy-pasting

eccb78b

kangax reviewed Jan 10, 2017

View reviewed changes

boopathi added 2 commits January 11, 2017 10:57

Add more tests

cf59b05

Use Symbol for EVAL_SCOPE_MARKER

b33897d

kangax reviewed Jan 17, 2017

View reviewed changes

kangax requested changes Jan 17, 2017

View reviewed changes

boopathi added the Tag: Bug Fix Pull Request fixes a bug label Jan 19, 2017

boopathi added 2 commits January 19, 2017 17:38

Merge branch 'master' into dce-eval-0

39a30f1

Fix lint

9bf6f64

kangax approved these changes Jan 19, 2017

View reviewed changes

kangax merged commit 2b98b98 into master Jan 19, 2017

kangax deleted the dce-eval-0 branch January 19, 2017 21:02

boopathi mentioned this pull request Jan 31, 2017

Reuse removed vars in mangler #395

Merged

8 tasks

targos mentioned this pull request Feb 16, 2017

babili is removing function arguments in scope of eval call #366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mark eval scopes helper and deopt DCE fn unused params #371

Add mark eval scopes helper and deopt DCE fn unused params #371

boopathi commented Jan 10, 2017 •

edited

boopathi commented Jan 10, 2017

boopathi commented Jan 10, 2017

kangax Jan 10, 2017

boopathi Jan 11, 2017

kangax Jan 10, 2017

boopathi Jan 10, 2017

kangax left a comment

boopathi commented Jan 11, 2017

kangax commented Jan 13, 2017

kangax Jan 17, 2017

boopathi Jan 17, 2017

boopathi Jan 17, 2017

kangax Jan 17, 2017

boopathi Jan 18, 2017

kangax left a comment •

edited

kangax commented Jan 19, 2017

kangax commented Jan 19, 2017

		@@ -0,0 +1,52 @@
		"use strict";

		const EVAL_SCOPE_MARKER = "evalInScope";

Add mark eval scopes helper and deopt DCE fn unused params #371

Add mark eval scopes helper and deopt DCE fn unused params #371

Conversation

boopathi commented Jan 10, 2017 • edited

boopathi commented Jan 10, 2017

boopathi commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kangax left a comment

Choose a reason for hiding this comment

boopathi commented Jan 11, 2017

kangax commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kangax left a comment • edited

Choose a reason for hiding this comment

kangax commented Jan 19, 2017

kangax commented Jan 19, 2017

boopathi commented Jan 10, 2017 •

edited

kangax left a comment •

edited