dead code not eliminated when two Boolean.parseBoolean in same function

[arenevier]

Hi,

In some circumstances, unreachable code is not removed properly.

I am adding a minimal reproducible testcase:

package com.google.gwt.sample.hello.client;
    
import com.google.gwt.core.client.EntryPoint;
import com.google.gwt.user.client.Window;

abstract class BaseDelegate {
  abstract public void display();
} 

class DelegateFoo extends BaseDelegate {
  @Override
  public void display() {
    Window.alert("Foo");
  }
} 
  
class DelegateNoFoo extends BaseDelegate {
  @Override
  public void display() {
    Window.alert("No Foo");
  }
}
  
public class Hello implements EntryPoint {
  static boolean isFeatureFooEnabled() {
    if (Boolean.parseBoolean(System.getProperty("myapp.property1"))) {
      return true;
    } 
    if (Boolean.parseBoolean(System.getProperty("myapp.property2"))) {
      return true;
    }
    return false;
  }

  public void onModuleLoad() {
    final BaseDelegate delegate = isFeatureFooEnabled() ? new DelegateFoo() : new DelegateNoFoo();
    delegate.display();
  }
} 

If myapp.property1 and myapp.property2 are both set to false, the above code will compile to

  delegate = ($clinit_Boolean() , $clinit_Boolean() , false)?new DelegateFoo:new DelegateNoFoo;
  delegate.display();

So it means that even if DelegateNoFoo is never used, its code will not be removed. It's a constraint example, but in our application, those delegates are actually pretty big and depend on many other classes. It means that we end up building and shipping a lot of unused code.

If there is only one Boolean.parseBoolean, the code compiles (correctly) to
delegate = ($clinit_Boolean() , new DelegateNoFoo);

Also, if we modify isFeatureFooEnabled to

  static boolean isFeatureFooEnabled() {
    if (Boolean.parseBoolean(System.getProperty("myapp.property1"))) {
      return true;
    }
    return Boolean.parseBoolean(System.getProperty("myapp.property2"));
  }

It will compile to delegate = ($clinit_Boolean() , $clinit_Boolean() , new DelegateNoFoo);

But this is fragile and won't scale if isFeatureFooEnabled() becomes more complex

[jhickman]

The issue you’re having is due to the complexity of your isFeatureFooEnabled method in how the GWT compiler is able to inline everything and remove unreachable code.

I was able to take your provided example and started experimenting with it. Using the initial implementation

  static boolean isFeatureFooEnabled() {
    if (Boolean.parseBoolean(System.getProperty("myapp.property1"))) {
      return true;
    } 
    if (Boolean.parseBoolean(System.getProperty("myapp.property2"))) {
      return true;
    }
    return false;
  }

If I set both properties to true, then it’s able to inline it perfectly and results in a simple:

$wnd.alert(‘Foo’);

And it also removes the class declarations, since those are really useless by this point.

If I set property1 to true and property2 to false, then it results in this:

$wnd.alert(‘Foo’);

Same thing! But when setting property1 to false and property2 to true; then we see the results similar to what you provided:

delegate = ($clinit_Boolean() , $clinit_Boolean() , true)?new DelegateFoo:new DelegateNoFoo;
delegate.display();

I think the reason it’s doing this is because the isFeatureFooEnabled has a ‘short circuit’ behavior due to the separate if statements. If property1 is true, then it will short circuit that entire method and will ALWAYS return true; thus being able to collapse it all and inline the alert. However, if property1 is false and property2 is true.. yes, it’ll still essentially return true, but that first ‘if’ statement is there and is executed before it returns that true. And, since property1 is false, it’s providing a placeholder to the $clinit_Boolean() function, which is essentially the empty function.

It’s possible that GWT could do extra pass at cleaning this up, but there are ways of implementing code to ensure code is removed properly due to being unreachable.

If I were to refactor the isFeatureFooEnabled method to reduce the complexity and help the compiler identify this, I get:

static boolean isFeatureFooEnabled() {
  return (Boolean.parseBoolean(System.getProperty("myapp.property1")))
    ||  (Boolean.parseBoolean(System.getProperty("myapp.property2")));
}

Setting either or both properties to true now will end up getting what we’d expect; inlining of the $wnd.alert(‘Foo’)

It also means that changing both properties to false will now inline $wnd.alert(‘No Foo’)

In both cases, the compiler was able to remove the unreachable class declarations properly.

[niloc132]

The linked issue #9922 should comprehensively solve this for the case of boolean, but there are still other oddities that it would be good to improve.

Some history of how clinits have been handled:

• Originally, this was called DuplicateClinitRemover, and was a facet of the JsInliner pass (as JsDelegationRemover) when it was originally added, some lengthy discussion can be found at https://groups.google.com/g/Google-Web-Toolkit-Contributors/c/u6Gosn-Y930. e6cf637#diff-3b5db2437f519ea9669049e2bce65fba1756d5ad53abc7804b7b8336cc14a27cR70
• The AST got a bit more complex, and at some point instead of removing duplicate clinit nodes, they could be replaced by null literals, and a later pass through the js optimize loop would remove that.
• The enclosing class was soon renamed to JsInliner, and DuplicateClinitRemover itself was renamed to DuplicateXORemover (for "Duplicate eXactly Once remover"). Ostensibly this could serve to rewrite other functions that can only execute once, but as far as I can tell, that never actually happened.
• Years pass, and eventually DuplicateXORemover is not just moved out of the JsInliner, but entirely pulled from the optimization loop. It is renamed to DuplicateExecuteOnceRemover now that it is a top-level class - but crucially, still creates null nodes. This means that the null nodes can never be optimized out any more, and are just left in the finished compiled source. Presumably this small size regression doesn't bother anyone - and shouldn't have any more performance impact than duplicate ;s at runtime, but it is still a little gross. 4a7c0d2
• About a year later, the DuplicateClinitRemover name is restored, and other vague metadata in the AST again becomes specific to Java clinits.

At no time, as far as I can tell (except possibly the very first version, I didn't attempt to try it), would the ($clinit_Foo(), $clinit_Foo(), FooMethod()) expression ever be cleaned up to only invoking $clinit_Foo once - the logic was only concerned with "if you find clinits in a , expr, were any of them duplicates of earlier calls?", and never "was the left-most something not yet seen? Okay, mark it as seen, then check the right".

6cb7659#diff-89bee58f816ba770a5fdd213869c8e639c329afcd379c2e69426f21093119d93R112-R113

Since , is left-associative, the example above would first evaluate the top-level exp (left is a binary expression with a comma, right is FooMethod()), visit the left: $clinit_Foo(), $clinit_Foo(), find neither had been evaluated before, leave them intact, and only then look at the first $clinit_Foo itself as something that should not be marked as seen. The second is also "seen", but the visit for an invocation crucially doesn't actually check if the invocation itself was seen before.

Additionally, since this was moved out of JsInliner and out of the JS optimization loop itself, there will never be another optimization pass (in this case, JsStaticEval) to remove the null literals that were used as replacement nodes. Ironically, replacing a clinit with a null might actually make the output longer, replacing a() three chars with null four.

Some thoughts on improving these cases:

• Reintroduce DuplicateClinitRemover to the js optimization loop - this could be done conditionally (e.g. only if inlining had an impact).
  • As a subpoint, better metrics so we know when a pass is expensive or runs needlessly would help to keep an eye on perf issues - and could also better check size improvements.
• DuplicateClinitRemover could change to handle not just commas and JsExprStmts, but actually look for invocations and record them as they are encountered, skipping if already seen. This could be most simply done by expanding how commas are handled (since all clinits return void, clinits can only be in commas or in exprstmts), but we could also simplify to just watching invocations and ignore commas/exprstmts entirely (since they are non-branching - though binaryoperations are still something to handle in special ways for short-circuiting ops).
• Add tests that the clinit remover actually does what it says it does, since there are cases where it doesn't.
• Add metrics to track changes that might degrade output, and make a point to discuss tradeoffs in PRs, commit messages (4a7c0d2 is the main one I'm looking at here).
• Explore eagerly initializing selected classes #9922 - as above, this should entirely solve the specific issue this ticket was raised for - though we still want a general fix.

---

The first pass of DeadCodeElimination is rewriting the isFeatureFooEnabled method (in the false, true case that Justin identified) roughly to

static boolean isFeatureFooEnabled() {
  clinit_Boolean();
  clinit_Boolean();
  return false;
}

Then, MethodInliner tries to address this - it rewrites it to a "multi-expression" (which doesn't exist in Java, but our GWT AST supports it because we can nicely generate it in JS). Inlining methods is limited to single-statement calls with no switch-expressions in them, and has the further limitation that no more than 2 expressions are allowed to exist in this expression.

gwt/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java

Lines 458 to 478 in 2b6f856

	private static boolean isTooComplexToInline(List<JExpression> bodyAsExpressionList,
	boolean ignoringReturn) {
	/*
	* Limit inlined methods to multiexpressions of length 2 for now. This
	* handles the simple { return JVariableRef; } or { expression; return
	* something; } cases.
	*
	* TODO: add an expression complexity analyzer.
	*/
	if (bodyAsExpressionList.size() > 3) {
	return true;
	}
	if (bodyAsExpressionList.size() == 3
	&& (!ignoringReturn || bodyAsExpressionList.get(2).hasSideEffects())) {
	return true;
	}
	// The expression is effectively of size 2, hence not too complex to inline.
	return false;
	}

Unfortunately, the multi-expression we build here has 3 expressions, clinit_Boolean(), clinit_Boolean(), true. As such, we are denied the ability to inline this as a java method. If #9922 was fixed, we would only have true, if we added a DuplicateClinitRemover for the Java ast, we would only have one, and this would have been inlined, and the extra class removed.

We could also explore raising that limit, but I don't yet understand why it is there, so that seems risky. Likely this prevents larger, more complex methods from being inlined and bloating code size (at least until another pass simplifies them, and then they can be inlined).

Our next shot is in JsInliner, which has different rules about what it can inline - it is more limited in what it can look at, but as a result it doesn't care quite as much about the contents. It agrees to inline the "isFeatureFooEnabled" method into the caller, but takes with it both clinits.

This gets us now to

($clinit_Boolean() , $clinit_Boolean() , true) && ($wnd.alert('Feature Foo is enabled') , undefined)

That undefined will get rewritten away (it was picked up by inlining in the Window.alert method), but that true turns out to be persistent. Our next rewrite would take that "short circuit AND" and notice that the left is always true... but since the left side is actually a JsBinaryOperation (using commas, but the actual operator doesnt matter for this), we ignore it.

gwt/dev/core/src/com/google/gwt/dev/js/JsStaticEval.java

Lines 612 to 632 in 2b6f856

	/**
	* Simplify short circuit AND expressions.
	*
	* <pre>
	* if (true && isWhatever()) -> if (isWhatever()), unless side effects
	* if (false() && isWhatever()) -> if (false())
	* </pre>
	*/
	protected static JsExpression shortCircuitAnd(JsBinaryOperation expr) {
	JsExpression arg1 = expr.getArg1();
	JsExpression arg2 = expr.getArg2();
	if (arg1 instanceof CanBooleanEval) {
	CanBooleanEval eval1 = (CanBooleanEval) arg1;
	if (eval1.isBooleanTrue() && !arg1.hasSideEffects()) {
	return arg2;
	} else if (eval1.isBooleanFalse()) {
	return arg1;
	}
	}
	return expr;
	}

To fix that, we finally have a real, actionable change we could make that is specific to this bug - we could investigate further, either pushing the right side into the last item in the left (since this is a , operator, and all will be evaluated), or move the first N-1 items to their own, earlier statement (this only works if the current node isn't nested in something that could be short circuited). The Java AST has passes that do each of these:

gwt/dev/core/src/com/google/gwt/dev/jjs/impl/Simplifier.java

Lines 222 to 240 in 2b6f856

	/**
	* Simplifies an if then else statement.
	*
	* <pre>
	* if(a,b,c) d [else e] -> {a; b; if(c) d [else e]; }
	* if(true) a [else b] -> a
	* if(false) a else b -> b
	* if(notImpl(c)) a else b -> if(c) b else a
	* if(true) ; else b -> true
	* if(false) a [else ;] -> false
	* if(c) ; [else ;] -> c
	*</pre>
	*
	* @param ifStatement the statement to simplify.
	* @param methodReturnType the return type of the method where the statement resides if any
	* @return the simplified statement if a simplification could be done and <code>ifStatement</code>
	* otherwise.
	*/
	public static JStatement simplifyIfStatement(JIfStatement ifStatement, JType methodReturnType) {

gwt/dev/core/src/com/google/gwt/dev/jjs/impl/DeadCodeElimination.java

Lines 162 to 172 in 2b6f856

	if (isNonEmptyMultiExpression(lhs)) {
	// Push the operation inside the multiexpression. This exposes other optimization
	// opportunities for latter passes e.g:
	//
	// (a(), b(), 1) + 2 ==> (a(), b(), 1 + 2) ==> (a(), b(), 3)
	//
	// There is no need to consider the symmetric case as it requires that all expression in the
	// rhs (except the last one) to be side effect free, in which case they will end up being
	// removed anyway.
	List<JExpression> expressions = ((JMultiExpression) lhs).getExpressions();

Lastly, this doesn't apply in the case I created, but it does apply to the original

gwt/dev/core/src/com/google/gwt/dev/jjs/impl/Simplifier.java

Lines 151 to 168 in 2b6f856

	/**
	* Simplify conditional expressions.
	*
	* <pre>
	* (a,b,c)?d:e -> a,b,(c?d:e)
	* true ? then : else -> then
	* false ? then : else -> else
	* cond ? true : else) -> cond || else
	* cond ? false : else -> !cond && else
	* cond ? then : true -> !cond || then
	* cond ? then : false -> cond && then
	* !cond ? then : else -> cond ? else : then
	* </pre>
	*
	* @param expression a JCondintional to be simplified.
	* @return the simplified expression if a simplification was possible; <code>exp</code> otherwise.
	*/
	public static JExpression simplifyConditional(JConditional expression) {

Note that it isn't a given that this would actually optimize out the unused class, though it does seem likely that it would get the bulk of it at least.

---

This ticket should probably be split in two (or three) to address these pieces individually:

• improve JS handling of clinits, ensure that we clean up what we used to clean, and additionally dedup clinits within comma exprs, see the bullet points above.
• investigate the MethodInliner restrictions, see if there are any good ways to let us inline a little sooner, so that Java opt5im
• JsStaticEval should peek inside BinaryExpressions with commas when evaluating ifs/conditionals (and possibly others) to rearrange children with side effects (such as clinits) so that they don't interfere with rewriting the rest of the expression.