Trivial dead code elimination in residual functions #1061

bakkot · 2017-10-06T21:27:21Z

Progress towards #632.

There's a lot more that can be done here - for example, we could also eliminate if (0) foo() - but presumably that should be done by partial evaluators in a less ad-hoc way.

facebook-github-bot · 2017-10-06T21:27:27Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2017-10-06T22:10:22Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

NTillmann

Looks great! I added some comments with suggestions (another one: eliminate while (false) { ... }). There's only one thing I'd really like to see: One more test case for the third case in the handling of IfStatements.

NTillmann · 2017-10-06T22:34:59Z

src/serializer/visitors.js

+      let node = path.node;
+      if (t.isBooleanLiteral(node.test)) {
+        if (node.test.value) {
+          // Strictly speaking this is not safe: Annex B.3.4 allows FunctionDeclarations as the body of IfStatements in sloppy mode,


I like your comment.

NTillmann · 2017-10-06T22:36:00Z

src/serializer/visitors.js

+  IfStatement: {
+    exit: function(path: BabelTraversePath, state: ClosureRefReplacerState) {
+      let node = path.node;
+      if (t.isBooleanLiteral(node.test)) {


You could generalize all cases a bit for other kinds of literals that have truthy values.

Sure, done. This is getting further and further into the realm of partial evaluation, though - for example, I'm considering [] to be truthy, but not [0] (because I don't want to eliminate [sideEffect()], and don't have a 'isPossiblySideEffecting' function on hand).

NTillmann · 2017-10-06T22:37:49Z

test/serializer/optimizations/residualDeadCode.js

+  let t = true;
+  global.inspect = function () {
+    if (f) { console.log("eliminate me"); }
+    if (t) { } else { console.log("eliminate me"); }


I see three cases in your handling of the IfStatement above, but only two are getting exercised here.

Added a test. I'm not super happy with these tests, though - there doesn't seem to be a good way of asserting the precise shape of the output. For example, I can assert that [] || 'eliminate' prepacks into something not containing 'eliminate', but not that it actually ends up as [], which is an important property to preserve.

Have you considered adding snapshot tests? For example, the prettier project uses them almost exclusively, to great effect. It makes it easy to see what the impact of a change is over a large range of code.

NTillmann · 2017-10-06T22:39:46Z

src/serializer/visitors.js

+  LogicalExpression: {
+    exit: function(path: BabelTraversePath, state: ClosureRefReplacerState) {
+      let node = path.node;
+      if (node.operator === "&&" && t.isBooleanLiteral(node.left, { value: false })) {


Not very exciting, but you could also reduce true && x to x, and false || x to x.

I didn't add any tests for this, mind, because I didn't see a good way to fit it into the existing test framework. Same for reducing do { foo } while (false) to { foo }.

hermanventer

Looks good to me.

NTillmann · 2017-10-07T00:31:43Z

src/serializer/visitors.js

@@ -56,6 +56,27 @@ function replaceName(path, residualFunctionBinding, name, data) {
  }
 }

+function getLiteralTruthiness(node) {
+  // Returns { known: boolean, value?: boolean }. 'known' is true only if this is a literal of known truthiness and with no side effects; if 'known' is true, 'value' is its truthiness.


No need to hide type info in comment, you can make it a real type annotation :-)

To avoid costly object allocations, a simpler scheme of void | false | true would be sufficient.

I generally figure the cost of this sort of object allocation is worth the additional readability, at least in modern engines. v8 at least is pretty good at optimizing the creation and use of this sort of object.

NTillmann · 2017-10-07T00:38:34Z

src/serializer/visitors.js

+  if (t.isNullLiteral(node)) {
+    return { known: true, value: false };
+  }
+  return { known: false };


Another case you could add: If the node is a unary expression with operator void and the argument is a literal, then this node will evaluate to undefined without side effects, and thus results in the known truthiness value of false.

This is true. I could also handle !false, +0, [0, true], void void [ +!'a', { foo: 0, get bar(){ return whatever(); } } ], typeof /a/, and so on. But one could go a long, long way down this road, and I think it probably makes sense to do the remainder of that work as part of partial evaluation, rather than in an ad-hoc way here.

NTillmann

Looks great!

All my remaining comments are very optional, so I'll try merge in your changes soon when the CI succeeds.

bakkot · 2017-10-07T00:57:42Z

Ah, one sec, just realized I overlooked a case...

ghost · 2017-10-07T00:58:51Z

help me

ghost · 2017-10-07T01:00:40Z

Getting started.pdf

bakkot · 2017-10-07T01:02:44Z

OK, updated. Should be good to go.

I forgot (class { [foo()](){} } exists, and might be side-effecting, which makes classes with bodies not safe to treat as literals.

There is an absurd number of expression positions in JavaScript.

jeffreytan81 · 2017-10-09T18:15:57Z

src/serializer/visitors.js

+    },
+  },
+
+  DoWhileStatement: {


For completeness/code coverage, please also add a testcase for "do...while".

I'm not sure how. I don't know what assertion I can make about them, given the current test framework.

(I'd love to have snapshot tests...)

Maybe just use "does not contain: while" for now.

Sure, done.

facebook-github-bot

@NTillmann is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Progress towards #632. There's a lot more that can be done here - for example, we could also eliminate `if (0) foo()` - but presumably that should be done by partial evaluators in a less ad-hoc way. Closes #1061 Differential Revision: D6012613 Pulled By: NTillmann fbshipit-source-id: a7dd6b64a42be34dddf1a59f363ed101feefda64

Summary: Followup to #1061; I thought of another case where it was too aggressive. Do-while loops with always-false conditions can't be replaced by their body because their body might contain a break or continue, which is illegal (or will have different semantics) if the loop becomes a normal block. In principle the body of the loop could be walked to find those, but frankly I doubt it will come up enough to be worth it. Closes #1145 Differential Revision: D6303312 Pulled By: hermanventer fbshipit-source-id: 0d677076fea6f225573a36fbbc9df1a0bebd959a

Trivial dead code elimination in residual functions

19353de

facebook-github-bot added the CLA Signed label Oct 6, 2017

NTillmann suggested changes Oct 6, 2017

View reviewed changes

hermanventer approved these changes Oct 6, 2017

View reviewed changes

bakkot added 3 commits October 6, 2017 17:01

Eliminate more literals

0c9e79c

Add test

c5cc75b

Also eliminate trivial loops

f3643ce

bakkot force-pushed the dead-code-elimination branch from a7567aa to f3643ce Compare October 7, 2017 00:20

NTillmann reviewed Oct 7, 2017

View reviewed changes

Move type annotation

f07e9f0

NTillmann approved these changes Oct 7, 2017

View reviewed changes

Do not eliminate classes for which declaration might be side effecting

86dd04e

jeffreytan81 approved these changes Oct 9, 2017

View reviewed changes

Add do-while test

4f66642

facebook-github-bot reviewed Oct 9, 2017

View reviewed changes

facebook-github-bot closed this in 93b6c36 Oct 9, 2017

bakkot deleted the dead-code-elimination branch October 9, 2017 21:48

bakkot mentioned this pull request Nov 10, 2017

Do not dead-code eliminate do-while in residual fns #1145

Closed

NTillmann mentioned this pull request Feb 28, 2018

Do some constant-folding / dead-code elimination in residual functions #632

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trivial dead code elimination in residual functions #1061

Trivial dead code elimination in residual functions #1061

bakkot commented Oct 6, 2017

facebook-github-bot commented Oct 6, 2017

facebook-github-bot commented Oct 6, 2017

NTillmann left a comment

NTillmann Oct 6, 2017

NTillmann Oct 6, 2017

bakkot Oct 7, 2017

NTillmann Oct 6, 2017

bakkot Oct 7, 2017

NTillmann Oct 6, 2017

bakkot Oct 7, 2017

bakkot Oct 7, 2017

hermanventer left a comment

NTillmann Oct 7, 2017

NTillmann Oct 7, 2017 •

edited

Loading

bakkot Oct 7, 2017

NTillmann Oct 7, 2017

bakkot Oct 7, 2017

NTillmann left a comment

bakkot commented Oct 7, 2017

ghost commented Oct 7, 2017

ghost commented Oct 7, 2017

bakkot commented Oct 7, 2017

jeffreytan81 Oct 9, 2017

bakkot Oct 9, 2017

jeffreytan81 Oct 9, 2017

bakkot Oct 9, 2017

facebook-github-bot left a comment

Trivial dead code elimination in residual functions #1061

Trivial dead code elimination in residual functions #1061

Conversation

bakkot commented Oct 6, 2017

facebook-github-bot commented Oct 6, 2017

facebook-github-bot commented Oct 6, 2017

NTillmann left a 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

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

hermanventer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTillmann Oct 7, 2017 • edited Loading

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

NTillmann left a comment

Choose a reason for hiding this comment

bakkot commented Oct 7, 2017

ghost commented Oct 7, 2017

ghost commented Oct 7, 2017

bakkot commented Oct 7, 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

facebook-github-bot left a comment

Choose a reason for hiding this comment

NTillmann Oct 7, 2017 •

edited

Loading