Simplify boolean expressions

[zbynek]

In the output I noticed statements like

!!a&&a.b();

which can be simplified to

a&&a.b();

In general, if we know that we ignore the result of an evaluation or we know the result is coerced to a boolean, we can apply more simplifications.

---

Other examples from compiler output this should simplify:

a.K==(Slh(),Klh)&&false?(n.g+=TWo,n):tNg(a,n,g)
O7d(e.yb)&&Tde(e.Qb,e.yb)&&false?Tqe(e):Pqe(e)

[niloc132]

I like the patch - it seems to go a little beyond what is discussed in the description though. Can you describe a bit more how the JS is being generated that causes this - is it from Java that isn't handled correctly, or is this JSNI (or jsinterop-base's JSNI) emitting these extra !!s? Might make sense to have an issue to generally track the topic too, in case there are more examples of it.

[niloc132]

would you document the meaning of these, and what it means for evalContext to not have a key at all?

My understanding from (heh) context is that VOID stands for "value will be thrown away", and BOOL means "this will be coerced to boolean", while absent means we don't know, so retain everything

[niloc132]

I don't think this is the right place for this - it should be part of the JsStaticEval check. the comments below indicate that the converse is "already simplified", but that's only true post-JsStaticEval.

The same code that does the shortCircuitOr()/etc check should also be looking for this case - you added logic to shortCircuitAnd, does it not work for OR too?

If we need this here, we probably also need to test for , operations... those are messier since they imply side effects on the LHS (JsStaticEval.trySimplifyComma should remove those though).

DeadCodeElimination/DCE and Simplifier do another transformation that doesnt appear to happen in JsStaticEval - expressions like (a,b)?c:d are rewritten to (a, b?c:d) so that b can be statically eval'd by the Conditional. If you're running into a situation where JsStaticEval can't work out the value of a sub-expr, it could be from something like this?

DCE at least has some checks in tests to assert that it only runs once and always converges - JsStaticEval is written in a similar style, but doesn't have such a check.

[zbynek]

I don't think this is the right place for this

I wasn't sure, the JsPrefixOperation has this, so having it in binary operations seemed consistent. But I didn't want an implementation that checks the whole tree and you're right that what I cam up was only correct under the assumption that some simplifications are already done, which is too fragile. So I now went with your approach of replacing && and || with , and adding a check for , in conditionals.

The same code that does the shortCircuitOr()/etc check should also be looking for this case

I fixed the code to be symmetric now.

[niloc132]

Using #10263 to bring this up to date, I show that this brings samples down from 10821121 to 10800491, some 20kb, or about 0.2% - not bad for some light logic improvements!

[niloc132]

Taking a little more time to go over this, unless this is exclusively a JSNI problem I think you really want this to be moved/copied to DeadCodeElimination. Doing an optimization on constants like this earlier is extremely likely to benefit the Java optimization loop more than the JS loop (where JsStaticEval lives), as removing flow control is quite likely to also improve inlinability and enhance type tightening or pruning - something that the JS passes are unable to reason about as effectively.

I'm nearly certain the two samples at the end of the description are Java rather than JSNI:

a.K==(Slh(),Klh)&&false?(n.g+=TWo,n):tNg(a,n,g)

Slh is almost certainly a Java clinit, which cannot be pruned (tests would need to show that it is promoted), and n is probably a stringbuilder/stringbuffer, getting a constant appended to it. DCETest has a few other cases like this that are verified, writing such a test is a little harder to do on a JS AST, since you can't just parse the JS, but need to visit the JS AST and edit it after the fact, and mark them as such - com.google.gwt.dev.js.JsDuplicateFunctionRemoverTest#setAllFromJava has an example of that, and I have a PR to land eventually for #9731 that does this too...

O7d(e.yb)&&Tde(e.Qb,e.yb)&&false?Tqe(e):Pqe(e)

Harder to be sure about anything here, but I'd bet that Tqe and Pqe are Java instance methods on e that were made static. If true, also implies that the java optimization loop is where this would do its best work.

Keeping the change also in the JS loop isn't bad per se, as it will still apply to JSNI or any other work that the Java loop can't optimize out. I could easily imagine that the (apparently uninlinable) pruned instance methods could have other impacts on code size that the JS loop can't necessarily recognize.

[zbynek]

So there are two issues with the boolean expressions

1. extraneous !! operators. In the compiled code I checked (~7MB) there were over 5 thousand !! operators and more than a half of them by random inspection seemed redundant. These are not JSNI and do not exist in Java code, they are created during translation from Java to JS. So JsStaticEval sounds like the right place to fix them (other occurences of !! are already eliminated by JsStaticEval.
2. dead code not eliminated from e.g. a() && false && b()?c():d(). In the same codebase I found 36 occurences of &&false, 2 of &&true, 1 of ||false, so this is a much smaller problem but some of the eliminated bits might be big. In my last commit I made sure this is handled in DCE. For now I left the equivalent code also in JsStaticEval, but if there are concerns about increased complexity of that class, I can remove some of it.

[vjay82]

Might there be another reason to use !!, like to convert to boolean or something?

[niloc132]

I think the patch mostly looks good, just trying to understand the root issue, be sure we can't do less and get more from it. Nothing here seems like it could increase build times, and for how much we're saving already, it seems obviously worth it.

I think the !! is being added in EqualityNormalizer - all kinds of null/not-null checks are delegated to Cast.isNull/isNotNull, which are just JSNI for !src/!!src respectively, and JSNI methods are automatically eligible for the JS inlining operation (since java's MethodInliner couldn't touch them). Once inlined, JsStaticEval 400 or so should be handling them - but the issues you've discovered with booleans in the "middle" of &&s and ||s is the root cause in interrupting that?

Concrete example:

gwt/user/super/com/google/gwt/emul/java/util/TreeMap.java

Lines 828 to 834 in b7117c1

	/**
	* Returns true if <code>node</code> is red. Note that null pointers are
	* considered black.
	*/
	private boolean isRed(Node<K, V> node) {
	return node != null && node.isRed;
	}

This is inlined into removeWithState:
https://github.com/gwtproject/gwt/blob/b7117c157bbb3bcfcc022433e307722a8e12fb33/user/super/com/google/gwt/emul/java/util/TreeMap.java#L882C12-L882C17
Resulting in roughly

    if (!(!!node && node.isRed) && !$isRed(node.child[dir])) {

That !!node should definitely should be corrected, since it is unambiguously in a boolean context - your addition of visit(JsBinaryOperation,JsContext) should by itself correct this, since that will let the endVisit on the outer ! remove both:

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

Lines 399 to 418 in b7117c1

	/**
	* Change !!x to x in a boolean context.
	*/
	@Override
	public void endVisit(JsPrefixOperation x, JsContext ctx) {
	if (x.getOperator() == JsUnaryOperator.NOT) {
	evalBooleanContext.remove(x.getArg());
	}
	if (evalBooleanContext.contains(x)) {
	if ((x.getOperator() == JsUnaryOperator.NOT)
	&& (x.getArg() instanceof JsPrefixOperation)) {
	JsPrefixOperation arg = (JsPrefixOperation) x.getArg();
	if (arg.getOperator() == JsUnaryOperator.NOT) {
	ctx.replaceMe(arg.getArg());
	return;
	}
	}
	}
	}

So I have no doubts about the patch being right, I'm sorry if I've implied that. Adding the Java side to go after <expr> && false and friends in the earlier loop will improve that pass, and hopefully make at least some of the JS pass unnecessary - though clearly not the !! checks.

I have another review pass about half done, just trying to more deeply understand what's going on here. No draft comments so far on the actual contents of the patch, just the tests and one comment I think could be cleaner.

[niloc132]

Might there be another reason to use !!, like to convert to boolean or something?

Exactly - but in cases where we know it will already be coerced to boolean, the extra !! is unnecessary.

For example

alert(maybeAString || false)

will either print the string, or if "falsey", the value false. If you want true/false as a string, you would need to do

alert(!!maybeAString || false)

(this is dumb, since it should just be alert(!!maybeAString) - that's what the second set of changes in the patch addresses)

On the other hand

alert(maybeAString ? 1 : 2)

Here we don't need the !! "operator" to coerce to bool, since it can only be used in a boolean context.

[niloc132]

Sorry, taking longer than I had anticipated with the release going on.

Oddly, I'm seeing zero impact from this patch in the Java AST of the Showcase sample - I'm still trying to understand why. The original objective of cleaning up !!s is going to have to happen only in the JS AST, but the nested &&+?: examples still appear to be from Java, unless this is again just a case of JSNI method inlining. I'll keep poking, trying to find examples where we could move the work earlier.

---

gwt/user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java

Lines 150 to 169 in b7117c1

	// Outer div contains image, value, and children (when open)
	StringBuilder outerClasses = new StringBuilder(itemStyle);
	if (isOpen) {
	outerClasses.append(openStyle);
	}
	if (isRootNode) {
	outerClasses.append(topStyle);
	}
	boolean isSelected = (selectionModel != null && selectionModel.isSelected(value));
	String ariaSelected = String.valueOf(isSelected);
	if (isSelected) {
	outerClasses.append(selectedStyle);
	}
	// Inner div contains image and value
	StringBuilder innerClasses = new StringBuilder(itemStyle);
	innerClasses.append(itemImageValueStyle);
	if (isRootNode) {
	innerClasses.append(topImageValueStyle);
	}

This code is improved in JS by this patch, in ways that don't seem obvious right away - the COMMA operators are getting cleaned up due to lack of side effects on the right side.

     isOpen = $containsKey(openNodes.map, key);
     outerClasses = new StringBuilder(itemStyle);
-    isOpen && (outerClasses.string += openStyle , outerClasses);
-    isRootNode && (outerClasses.string += topStyle , outerClasses);
+    isOpen && (outerClasses.string += openStyle);
+    isRootNode && (outerClasses.string += topStyle);
     isSelected = !!selectionModel && selectionModel.isSelected(value);
     ariaSelected = '' + isSelected;
-    isSelected && (outerClasses.string += selectedStyle , outerClasses);
+    isSelected && (outerClasses.string += selectedStyle);
     innerClasses = new StringBuilder(itemStyle);
     innerClasses.string += itemImageValueStyle;
-    isRootNode && (innerClasses.string += topImageValueStyle , innerClasses);
+    isRootNode && (innerClasses.string += topImageValueStyle);
     isOpen?(image = openImage):model.isLeaf(value)?(image = ($clinit_CellTreeNodeView() , LEAF_IMAGE)):(image = closedImage);
     cellBuilder = new SafeHtmlBuilder;

Not all cases of the "return this" are impacted by this, but these improvement sure seem like obvious wins, and occur in many other places in the showcase.

Another example here, where we get a small win, but due to apparent side effects in the clinit, we can't outright remove the whole line:

gwt/user/src/com/google/gwt/user/client/ui/RootPanel.java

Lines 191 to 201 in b7117c1

	// Note that the code in this if block only happens once -
	// on the first RootPanel.get(String) or RootPanel.get()
	// call.
	if (rootPanels.size() == 0) {
	// If we're in a RTL locale, set the RTL directionality
	// on the entire document.
	if (LocaleInfo.getCurrentLocale().isRTL()) {
	BidiUtils.setDirectionOnElement(getRootElement(),
	HasDirection.Direction.RTL);
	}
	}

   if (!!rp) {
     return rp;
   }
-  $size(rootPanels) == 0 && ($clinit_LocaleInfo() , false);
+  $size(rootPanels) == 0 && $clinit_LocaleInfo();
   rp = new RootPanel$DefaultRootPanel;
   $put(rootPanels, null, rp);

Here's a case where we probably should be removing the !! on the LHS, but instead leave it in place, possibly because this is a VOID context rather than BOOLEAN? Oddly, other cases like this are cleaned up. Maybe the code is too simple here, and being optimized in the Java ast so that we still see this pattern?

gwt/user/src/com/google/gwt/cell/client/ButtonCell.java

Lines 68 to 74 in b7117c1

	public void render(Context context, SafeHtml data, SafeHtmlBuilder sb) {
	sb.appendHtmlConstant("<button type=\"button\" tabindex=\"-1\">");
	if (data != null) {
	sb.append(data);
	}
	sb.appendHtmlConstant("</button>");
	}

 _.render = function render(context, data, sb){
   $append(sb.sb, '<button type="button" tabindex="-1">');
-  !!data && ($append(sb.sb, data.html) , sb);
+  !!data && $append(sb.sb, data.html);
   $append(sb.sb, '<\/button>');
 }

This next one is probably as optimized as this patch can get it, and shows how the old code was still getting some benefits of removing the dead code, just not as complete as what you worked out

gwt/user/src/com/google/gwt/user/datepicker/client/DatePicker.java

Lines 713 to 723 in b7117c1

	protected final void refreshAll() {
	highlighted = null;
	getModel().refresh();
	getView().refresh();
	getMonthSelector().refresh();
	if (isAttached()) {
	ShowRangeEvent.fire(this, getFirstDate(), getLastDate());
	}
	getView().setAriaSelectedCell(value);
	}

 function $refreshAll(this$static){
   $refresh(this$static.view);
   $refresh(this$static.monthAndYearSelector);
-  $isAttached(this$static) && this$static.view.lastDisplayed;
+  $isAttached(this$static);
   $setAriaSelectedCell(this$static.view, this$static.value);
 }

It is possible that this patch will open up other avenues for #10242 to benefit the codebase like this one.

This makes me mildly suspicious - is it correct to drop the || 0 here? This is a "number" context, right, since we're |ing the desired event bits against the existing ones?

gwt/user/src/com/google/gwt/user/client/ui/Widget.java

Lines 239 to 245 in b7117c1

	public void sinkEvents(int eventBitsToAdd) {
	if (isOrWasAttached()) {
	super.sinkEvents(eventBitsToAdd);
	} else {
	eventsToSink |= eventBitsToAdd;
	}
	}

 _.sinkEvents = function sinkEvents(eventBitsToAdd){
-  this.eventsToSink == -1?sinkEvents(($clinit_DOM() , this.element), eventBitsToAdd | ($clinit_DOM() , ($clinit_DOM() , this.element).__eventBits || 0)):(this.eventsToSink |= eventBitsToAdd);
+  this.eventsToSink == -1?sinkEvents(($clinit_DOM() , this.element), eventBitsToAdd | ($clinit_DOM() , ($clinit_DOM() , this.element).__eventBits)):(this.eventsToSink |= eventBitsToAdd);
 }

My read is that if you simplify it, this looks like

sinkEvents(this.element), eventBitsToAdd | (this.element.__eventBits || 0));

and so we want the ||0 as "coerce to number".

[niloc132]

I'm wondering if this comment is useful here, since left-associativity means that we would view the whole
a() && false && b()
as
(a() && false) && b()

and so we would be relying on the JMultiExpr instanceof above for step two, and the lhs is the JBooleanLiteral on step three? I think a comment in the test makes sense to spell out that we're going to get all three different optimizations in a single pass, but probably just simplify here to something like

Suggested change

	// if side effect, allow rewriting a() && false && b() -> (a(), false) && b()
	// -> (a(), false && b()) -> (a(), false)
	// if side effect, allow rewriting a() && false -> (a(), false)

Also technically (a(),false) is a character longer than a()&&false - perhaps we shouldn't touch it unless we know the current binaryoperation is itself within a binaryoperation?

[zbynek]

It may be tricky to know in advance if the outer bracket is needed, if it's not, the , expression is shorter.

alert(foo()&&false)
alert(foo(),false)

[niloc132]

Agreed - my "technically" responses are mostly to mean "this is possible, but probably not easy, and doesnt save us many bytes".

[zbynek]

I can reproduce the issue with ||0 in a test, checking...

[niloc132]

We don't usually need to add something like random() to trick the compiler in these tests, since very little of the compiler is run here, just DCE and MethodInliner (requested by this method, line 295) - specializer is still left disabled. It won't even notice that random() fails to return a value...

Given the tests you're adding, you might just prefer to make a new test method and leave the inliner off, so the compiler just lets you call a method and not try to inline it away? Then you could write a() && false && b() and ensure you only get (EntryPoint$A.a(),false) out, rather than "expect" that the random() and stuff gets inlined in?

Alternatively, JSNI can never be inlined in the Java ast, so you could also just write randomBooleanWithSideEffects() itself as jsni, and it will be assumed to have side effects, not be inlined away.

To be clear, what you have now isn't bad, but it does look like you may have been fighting the test setup a bit to get the test you wanted.

[niloc132]

Note that it isn't obvious if the first or second randomBooleanWithSideEffects() was inlined here - the code could be wrong and inlining the wrong one, but the test output is the same.

[niloc132]

This last one technically can be simplified further right? !!(a||b)

Might also be nice to get a test like the first two, but with !!:

    assertEquals("alert(!!a);", optimize("alert(!!a || false)"));
    assertEquals("alert(true);", optimize("alert(!!a || true)"));

[niloc132]

EDIT: nope, wrong, ignore

[niloc132]

Question, is this passing because it is in a void context, the value is being thrown away? Is that the goal of the test, or should this be wrapped in something (like alert()) - or should the a also be optimized out?

[niloc132]

This doesn't look right - evalContext.containsKey(x) may or may not be false, so we could be putting null for the RHS's context, which means that if we read evalContext.containsKey(RHS) later, we might get "true" when we want "false".

[niloc132]

I worked backwards from one of the <expr> && false JS cases to see what was causing them, and it seems that your Simplifier changes are at least documented as dealing with this exact case - but at least in my testing, failing to do so.

First, the method that everything is inlined into - ignore everything except the super call at the end

gwt/user/src/com/google/gwt/user/client/ui/DialogBox.java

Lines 517 to 529 in b7117c1

	protected void onPreviewNativeEvent(NativePreviewEvent event) {
	// We need to preventDefault() on mouseDown events (outside of the
	// DialogBox content) to keep text from being selected when it
	// is dragged.
	NativeEvent nativeEvent = event.getNativeEvent();
	if (!event.isCanceled() && (event.getTypeInt() == Event.ONMOUSEDOWN)
	&& isCaptionEvent(nativeEvent)) {
	nativeEvent.preventDefault();
	}
	super.onPreviewNativeEvent(event);
	}

That super call is PopupPanel.onPreviewNativeEvent:

gwt/user/src/com/google/gwt/user/client/ui/PopupPanel.java

Lines 1046 to 1052 in b7117c1

	protected void onPreviewNativeEvent(NativePreviewEvent event) {
	// Cancel the event based on the deprecated onEventPreview() method
	if (event.isFirstHandler()
	&& !onEventPreview(Event.as(event.getNativeEvent()))) {
	event.cancel();
	}
	}

which in turn calls onEventPreview (as noted, this is deprecated, so just has a default impl):

gwt/user/src/com/google/gwt/user/client/ui/PopupPanel.java

Lines 704 to 707 in b7117c1

	@Deprecated
	public boolean onEventPreview(Event event) {
	return true;
	}

The !onEventPreview naturally will be inlined as just !true and at the end of the Java optimization loop, these are fully inlined, though we keep the event.nativeEvent accessor for some reason, but !true is now false

  protected final void onPreviewNativeEvent(Event$NativePreviewEvent event){
    final NativeEvent nativeEvent = event.nativeEvent;
    !event.isCanceled && Event.$getTypeInt((Event) event.nativeEvent) == 4 && DialogBox.$isCaptionEvent(this, nativeEvent) && DOMImplStandard.$eventPreventDefault(nativeEvent);
    event.isFirstHandler && (((Event) event.nativeEvent, false)) && (event.isCanceled = true);
  }

First iteration optimization loop with JS (before this patch's changes):

_.onPreviewNativeEvent = function onPreviewNativeEvent(event){
  var nativeEvent;
  nativeEvent = event.nativeEvent;
  !event.isCanceled && $getTypeInt(event.nativeEvent) == 4 && $isCaptionEvent(this, nativeEvent) && (nativeEvent.preventDefault() , undefined);
  event.isFirstHandler && false && (event.isCanceled = true);
}
;

The JS pass got the event.nativeEvent removal right, and then your patch correctly rewrites away the rest of the code in that line (event.isFirstHandler has no side effects, and the assignment will never be called). BUT, if Java correctly cleaned up that multiexpression (which your current DCE changes are attempting to do?). The issue is the JCastOperation - except when casting to primitive, these always are marked as having a side effect (since there could be a CCE thrown).

gwt/dev/core/src/com/google/gwt/dev/jjs/ast/JCastOperation.java

Lines 53 to 63 in b7117c1

	public boolean hasSideEffects() {
	if (castType.isPrimitiveType()) {
	// Primitive casts do not throw and only have side effects if the expression has side effects.
	return expr.hasSideEffects();
	}
	// Any live non-primitive cast operations might throw a ClassCastException
	//
	// TODO: revisit this when we support the concept of whether something
	// can/must complete normally!
	return true;
	}

At the same time, Event.as() is marked as "this is always safe"

gwt/user/src/com/google/gwt/user/client/Event.java

Lines 464 to 471 in b7117c1

	/**
	* Converts the {@link NativeEvent} to Event. This is always safe.
	*
	* @param event the event to downcast
	*/
	public static Event as(NativeEvent event) {
	return (Event) event;
	}

It turns out that the compiler will make the cast be free... because both Event and NativeEvent are JSOs. Event is a subclass of NativeEvent, so TypeTightener doesnt believe it is automatically safe, but really, since it is a JSO, it shouldn't check, it should just make it a free operation.

As for why it is a free cast once we're in JS:
First, casting a JSO to a JSO is ignored by the visitor that rewrites casts to Cast.java method calls

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

Lines 100 to 104 in b7117c1

	} else if (program.typeOracle.isEffectivelyJavaScriptObject(argType)
	&& program.typeOracle.isEffectivelyJavaScriptObject(refType)) {
	// leave the cast instance for Pruner/CFA, remove in GenJSAST
	return;
	}

Then, when we translate the Java AST to JS, we just ignore that leftover cast:

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

Lines 605 to 608 in b7117c1

	public JsNode transformCastOperation(JCastOperation castOperation) {
	// These are left in when cast checking is disabled.
	return transform(castOperation.getExpr());
	}

---

So for this single case, my conclusion is that it is a bug for GWT to ignore the case during JS AST generation, when it could just as easily have tightened the cast away earlier and made all of this unnecessary.

So at least for this specific example, better casting logic would have prevented this