OptimizedOnlyCompilerSuite is never run, fails

[niloc132]

As far as I can tell, OptimizedOnlyCompilerSuite has never run as part of the ant build since it was introduced, it has always been in the gwt.junit.testcase.web.excludes list, which prevents it from running not only in draft mode but also prod tests. It was first introduced in 24de83e, and was in the excludes list from then on forward.

Only one test fails:

gwt/user/test/com/google/gwt/dev/jjs/optimized/JsOverlayMethodOptimizationTest.java

Lines 29 to 65 in a433a55

	public class JsOverlayMethodOptimizationTest extends OptimizationTestBase {
	@JsType(isNative = true, namespace = JsPackage.GLOBAL, name = "Object")
	private static class NativeType {
	private native int nativeMethod();
	@JsOverlay
	final boolean contains(String s1, String s2) {
	return s1.contains(s2);
	}
	@JsOverlay
	final int alwaysCallNative() {
	// NativeType.contains should get inlined by Java passes so the code below should be
	// statically evaluated to return "this".
	return contains("1234","1") ? this.nativeMethod() : 0;
	}
	}
	public static int callAlwaysCallNative(NativeType obj) {
	return obj.alwaysCallNative();
	}
	private static native String getGeneratedFunctionDefinition() /*-{
	return function() {
	@JsOverlayMethodOptimizationTest::callAlwaysCallNative(*)({});
	}.toString();
	}-*/;
	/**
	* Tests whether JsOverlay is inlined by the Java optimization passes rather than being just
	* devirtualized.
	*/
	public void testJsOverlayIsInlined() throws Exception {
	String functionDef = getGeneratedFunctionDefinition();
	assertFunctionMatches(functionDef, "({}.nativeMethod())");
	}

The contains() method is not statically evaluated as expected and the rest of alwaysCallNative ends up not being inlined - instead we keep the overlap method (despite only having one invocation, which should make it an obvious candidate for js inlining).

In the short term we could disable the test and enable the suite for non-draft "web" runs. Should also bisect to see when it started failing, if this points to something else being broken.

Testcase: testJsOverlayIsInlined took 0.001 sec
	FAILED
content: "rd({});" does not match pattern: \(\{\}\.nativeMethod(\(\))?\);? - expected: <true>, actual: <false>
junit.framework.AssertionFailedError: content: "rd({});" does not match pattern: \(\{\}\.nativeMethod(\(\))?\);? - expected: <true>, actual: <false>
	at com.google.gwt.dev.jjs.optimized.OptimizationTestBase.assertFunctionMatches(OptimizationTestBase.java:30)
	at com.google.gwt.dev.jjs.optimized.JsOverlayMethodOptimizationTest.testJsOverlayIsInlined(JsOverlayMethodOptimizationTest.java:62)
	at Unknown.@script in about:blank from(Unknown)

function rd(a){return '1234'.includes('1')?a.nativeMethod():0}

[niloc132]

Reverting cfb4e79 fixes the failing test, which suggests it is a matter of the MethodInliner racing against the "this call is clearly something the compiler can statically evaluate" check in DeadCodeElimination. The new implementation is probably "too easy" to inline, so the test started failing.

The order of the passes in optimizeJavaToFixedPoint and MethodInliner running in a loop with a DeadCodeElimination pass after each attempt appears to be an effort to avoid exactly this case - before every chance for MethodInliner to rewrite any method, static eval should get a chance to spot the "1234".contains("1") and transform statically to true. Watching the compiler pass by pass it appears this is what is happening:

• initial state
• DCE can't statically evaluate contains("1234", "1")
• MethodInliner moves String.contains contents into the tests's NativeType.contains, so it now looks like

       final boolean contains(String s1, String s2) { 
         return s1.asNativeString().includes(s2);
       } 

• DCE can't do anything to this
• MethodInliner can again inline this into alwaysCallNative(), which now can never be transformed away from a ternary expression

Previously however, the inliner rewrote the old indexof implementation instead, resulting in something like

final boolean contains(String s1, String s2) {
  return s1.indexOf(s2) != -1;
}

and that in turn was inlined into alwaysCallNative() rather than having indexOf() itself be inlined - in short, we got lucky.

We should stick our thumbs on the scale a little more, and avoid this kind of luck where it prevents DCE from kicking in - String is a special class as far as DCE is concerned, and we should take greater pains to avoid inlining away real java.lang.String methods until we're quite confident that all other inlining and optimization has been performed. Something like a "@DoNotInlineQuiteSoAggressively" on all String methods that just delegate to NativeString methods. This might look a little like how method specialization works, where we remove all specialization markers and ask the compiler to inline everything one more time.

I'm hesitant to land a fix for this right before a release for fear that it might have some other side effect, but it might make for a good point release fix if we find it improves compiled size.

[niloc132]

Given the context above, I'm going to limit this ticket to merely ensuring that this suite is run and disabling the failing test. The details of improving DCE will be moved to #10147.

[niloc132]

I was wrong above - it wasn't (just) about the order of the inline operations, it was that contains(CharSequence) just can't be constant folded using the old implementation at all, as CharSequence isnt one of the supported types that could be passed in. But, when it was rewritten to be "1234".indexOf("1".toString()), the "1".toString() could easily be rewritten to just a constant, and then "indexOf" could be constant folded.

We won't be able to constant fold an arbitrary CharSequence param, only string literals can be supported - but once we're sure its a string literal, there's nothing wrong with supporting CharSequence.

Still should be part of 10147, just wanted to call out the mistake in case someone else tries to follow my notes here (like future me).