Allow JsUtils.uncheckedCast to be inlined away

[niloc132]

This will have minimal impacts in cases where JS inlining and static eval passes are able to make improvements, but will improve cases where Java/GWT passes can benefit from this method being removed, such as removing clinits.

The new implementation is effectively what jsinterop-base's Js.uncheckedCast() does, so is unlikely to cause any surprises.

Fixes #10055

[niloc132]

Size changes in the included samples built by default are indeed meager - but it does depend on the app, and what other optimizations can take advantage of this improvement.

• 89 bytes saved in Hello, where permutations are about 36KB
• 618 bytes saved in DynaTable, where permutations are 113k-115KB
• 687 bytes saved in JSON, where permutations are 80-81KB (almost 1%!)
• 630 bytes saved in Mail, where permutations are 198-199KB
• 779 bytes saved in Showcase, where permutations are 232-260KB

[niloc132]

There are no tests for this, as I haven't yet worked out a good way to test it - perhaps a "full compile" test that lends itself well to being optimized away. I'm not sure this change really warrants a test, at least not of the complexity this might take to set up, but I will briefly give it a shot.

[niloc132]

Test failure seems to be around a hashmap.put("Content-Type", "text/x-gwt-rpc") call that rpc makes to the requestbuilder - for some reason that is being optimized to have the key be null rather than the expected "Content-Type", so the header is missing when the call gets to the server as part of the test wiring.

This suggests that some overzealous optimization might be taking place, but I haven't yet pinned down what it is. I don't think I made a mistake in this patch, unless somehow I needed a "this might not be null" hint? If so, why isn't jsinterop-base impacted by that?

My best guess so far is that it only applies to @SpecializeMethod-annotated methods. It looks like when using @UncheckedCast on a non-JSNI method, the TypeTightener gets more aggressive (right or wrong) about tightening types to null. Then a compiler pass or two later, the MethodCallSpecializer rewrites the map.put(key, value) call to map.putStringValue(key, value), but key has been tightened to always be null, so the data is lost.

[niloc132]

More debugging - my hunch continues to be that SpecializeMethod is niche enough that it isn't as hardened as it should be, and JSNI methods have defied optimization so that method specialization hasn't broken before. Looking at a diff of an AST dump before and after this patch, we can see a number of ways that code gets cleaned up a little quicker with the jsinterop-base impl:
Simpler code:

@@ -50125,4 +50120,4 @@
       len = batchEnd - batchStart;
-      final Object[] spliceArgs = JsUtils.uncheckedCast(src).slice(batchStart, batchEnd);
-      JsUtils.uncheckedCast(spliceArgs).splice(0, 0, new Object[] {JsUtils.uncheckedCast((double) destOfs), JsUtils.uncheckedCast((double) (false ? len : 0))});
+      final Object[] spliceArgs = src.slice(batchStart, batchEnd);
+      spliceArgs.splice(0, 0, new Object[] {(double) destOfs, (double) (false ? len : 0)});
       ArrayHelper.getSpliceFunction().apply(destArray, spliceArgs);

Earlier inlining:

@@ -48992,7 +48987,7 @@
       case "string": 
-      return String.$hashCode(JsUtils.uncheckedCast(o));
+      return String.$hashCode(o);
       case "number": 
-      return Double.$hashCode(JsUtils.uncheckedCast(o));
+      return (int) JsUtils.unsafeCastToDouble((InternalPreconditions.$clinit(), InternalPreconditions.checkCriticalNotNull(o), o));
       case "boolean": 
-      return Boolean.$hashCode(JsUtils.uncheckedCast(o));
+      return JsUtils.unsafeCastToBoolean((InternalPreconditions.$clinit(), InternalPreconditions.checkCriticalNotNull(o), o)) ? 1231 : 1237;
       default: 

Background for our bug: @SpecializeMethod is an internal annotation that lets the compiler pick a more specific method based on what types are actually used. It behaves a bit like Java's own dynamic dispatch, but the compiler gets to change override not just based on the actual types, but the types that it works out by optimizing. Methods can also be selected that have different names, rather than being required to have the same name.

In our case, AbstractHashMap has a few of these, the relevant one is for put:

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

Lines 211 to 216 in 554447e

	@SpecializeMethod(params = {String.class, Object.class}, target = "putStringValue")
	@Override
	public V put(K key, V value) {
	return key instanceof String
	? putStringValue(JsUtils.uncheckedCast(key), value) : putHashValue(key, value);
	}

This not only has a typecheck to pick an implementation to use, but the compiler will actually rewrite calls to this second method if the first arg is able to be proven to be a string:

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

Lines 285 to 292 in 554447e

	/**
	* Sets the specified key to the specified value in the stringMap. Returns the
	* value previously at that key. Returns <code>null</code> if the specified
	* key did not exist.
	*/
	private V putStringValue(String key, V value) {
	return key == null ? putHashValue(null, value) : stringMap.put(key, value);
	}

Relevant AST dump diff for the bug this exposed - first, we see that content is being inlined quicker. This means that there are no longer any references to $putStringValue (the static version of putStringValue - @SpecializeMethod automatically applies to static implementations too).

@@ -50890,3 +50885,3 @@
   public Object put(Object key, Object value){
-    return key instanceof String ? AbstractHashMap.$putStringValue(this, JsUtils.uncheckedCast(key), value) : InternalHashCodeMap.$put(this.hashCodeMap, key, value);
+    return key instanceof String ? key == null ? InternalHashCodeMap.$put(this.hashCodeMap, null, value) : InternalStringMap.$put(this.stringMap, key, value) : InternalHashCodeMap.$put(this.hashCodeMap, key, value);
   }

The ternary is a little silly here (if key isnt a string, then it can't be null, so the first branch can literally never be taken). The rest all makes sense - but since nothing else calls $putStringValue any more, its params are "tightened" to nulls:

```patch
@@ -66242,4 +66204,4 @@
 
-  private static final Object $putStringValue(AbstractHashMap this$static, String key, Object value){
-    return key == null ? InternalHashCodeMap.$put(this$static.hashCodeMap, null, value) : InternalStringMap.$put(this$static.stringMap, key, value);
+  private static final Object $putStringValue(AbstractHashMap this$static, null key, null value){
+    return InternalHashCodeMap.$put(this$static.hashCodeMap, null, value);
   }

(Also note that because it did "successfully" tighten to null, more of the method goes away. This might make the size changes suspect in my comment earlier).

The "right" answer here is probably to just remove the unused method, but I guess this serves as an intermediate step?

From the order of operations that the compiler does when optimizing, note that MethodCallSpecializer is before MethodInlining specific to prevent this case:

gwt/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java

Lines 1498 to 1506 in 554447e

	stats.add(Finalizer.exec(jprogram, optimizerCtx).recordVisits(numNodes));
	stats.add(MakeCallsStatic.exec(jprogram, options.shouldAddRuntimeChecks(), optimizerCtx)
	.recordVisits(numNodes));
	stats.add(TypeTightener.exec(jprogram, optimizerCtx).recordVisits(numNodes));
	stats.add(MethodCallTightener.exec(jprogram, optimizerCtx).recordVisits(numNodes));
	// Note: Specialization should be done before inlining.
	stats.add(MethodCallSpecializer.exec(jprogram, optimizerCtx).recordVisits(numNodes));
	stats.add(DeadCodeElimination.exec(jprogram, optimizerCtx).recordVisits(numNodes));
	stats.add(MethodInliner.exec(jprogram, optimizerCtx).recordVisits(numNodes));

Finalizer and MakeCallsStatic run first, which will give our static methods a matching static target once they are inlined later. For some reason, we're missing specializing from put to putStringValue on the first pass (both before and after this change), but on the second pass we can do it.

Both before and after end up with the same optimization for the original map.put() call, the last line of this:

   public static final void $setHeader(RequestBuilder this$static, String header, String value){
     StringValidator.throwIfEmptyOrNull("header", header);
     StringValidator.throwIfEmptyOrNull("value", value);
     this$static.headers == null && (this$static.headers = new HashMap());
     AbstractHashMap.$putStringValue(this$static.headers, header, value);
   }

But since in our "after" code, the key is ignored (assumed to always be null), the map doesn't contain the data where we expect.

The "just hack it" part of me wants to just rearrange the compiler passes, so that we tighten types after specialization, or only inline after a few rounds of the other compilation steps to prevent this (as inlining both exposes the uncheckedCast issue and removes the callsites for the specialized impl). The existing interrelationships between compiler passes are poorly documented as it stands though, as this bug seems to reinforce.

(With that said... reordering does fix this one test at least.)

[niloc132]

The fundamental issue is that we're changing the specialization before we get a chance to actually use it. This can safely be prevented by already using it. The base method starts off by using it, but then optimizations end up rewriting it away (specifically inlining, then type tightening to make it incompatible).

This is suggested in a comment for a test, but not outright asserted elsewhere that I can find:

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

Lines 44 to 48 in 7c36c89

	@SpecializeMethod(params = {String.class, Object.class}, target = "putString")
	public void put(K k, V v) {
	if (k instanceof String) {
	putString((String) k, v); // keeps putString from being pruned
	}

The fix then seems to be to forbid inlining specializations into their own base method. There are two facets to this - we need to handle not only "inline putStringValue into put), but also "inline $putStringValue into put". This latter case can happen when the compiler has produced staticified versions of both put and putStringValue (where $put contains a call then to $putStringValue), then inlined $put into put. Inlining $putStringValue into $put is handled by the first case.

This change does seem to suggest that after specializations are unmarked (when the optimization loop is finished), it makes sense to run the inliner again before pruning, I'll explore that next (adding it to the obvious location fails the compile).

I've also added explicit notes in @SpecializeMethod to indicate that decorated methods must call their specialized impl - this was already done in all places where it is used. I've also tweaked a few usages of @SpecializeMethod to use uncheckedCast to avoid an unnecessary type check (Java 16 instanceof patterns are the "right" fix for this, but they generate a bit more code, and can't be always turned on at this time).

Gains are down with the updated patch, about 100 bytes for everything except Hello, which is still 89 bytes.

[vjay82]

Just as a short feedback. In our project (compiled with -style OBFUSCATED, -optimize 9 and -XdisableCastChecking) the output even grows a little.
My suspicion is that for example "function u(a,b){return a.indexOf(b);}" becomes more efficient over time if used a lot (length of "u" instead of "indexOf"). Probably still a good thing to shift more workload from V8 to compression.
Aside from that, all tests passed in our project.

[niloc132]

Thanks! Inlining is tricky to get right, and GWT doesn't take into account the obfuscated length of the method names vs inlined JS native calls, though perhaps it should.

The big advantage of inlining of course is that it can often allow other optimizations later by specializing for the method it was inlined into - though in this case, it never will.

Note that -XdisableCastChecking is deprecated since 2.8.0, and jre.checks.checkLevel should be used instead (values are "NORMAL", "OPTIMIZED", and "MINIMAL"), see InternalPreconditions for more info

gwt/user/super/com/google/gwt/emul/javaemul/internal/InternalPreconditions.java

Lines 23 to 73 in 7c36c89

	/**
	* A utility class that provides utility functions to do precondition checks inside GWT-SDK.
	* <p>Following table summarizes the grouping of the checks:
	* <pre>
	* ┌────────┬─────────────────────────────────────────────────────┬───────────────────────────────┐
	* │Group │Description │Common Exception Types │
	* ├────────┼─────────────────────────────────────────────────────┼───────────────────────────────┤
	* │BOUNDS │Checks related to the bound checking in collections. │IndexOutBoundsException │
	* │ │ │ArrayIndexOutOfBoundsException │
	* ├────────┼─────────────────────────────────────────────────────┼───────────────────────────────┤
	* │API │Checks related to the correct usage of APIs. │IllegalStateException │
	* │ │ │NoSuchElementException │
	* │ │ │NullPointerException │
	* │ │ │IllegalArgumentException │
	* │ │ │ConcurrentModificationException│
	* ├────────┼─────────────────────────────────────────────────────┼───────────────────────────────┤
	* │NUMERIC │Checks related to numeric operations. │ArithmeticException │
	* ├────────┼─────────────────────────────────────────────────────┼───────────────────────────────┤
	* │TYPE │Checks related to java type system. │ClassCastException │
	* │ │ │ArrayStoreException │
	* ├────────┼─────────────────────────────────────────────────────┼───────────────────────────────┤
	* │CRITICAL│Checks for cases where not failing-fast will keep │IllegalArgumentException │
	* │ │the object in an inconsistent state and/or degrade │ │
	* │ │debugging significantly. Currently disabling these │ │
	* │ │checks is not supported. │ │
	* └────────┴─────────────────────────────────────────────────────┴───────────────────────────────┘
	* </pre>
	*
	* <p> Following table summarizes predefined check levels:
	* <pre>
	* ┌────────────────┬──────────┬─────────┬─────────┬─────────┬─────────┐
	* │Check level │ BOUNDS │ API │ NUMERIC | TYPE │CRITICAL │
	* ├────────────────┼──────────┼─────────┼─────────┼─────────┼─────────┤
	* │Normal (default)│ X │ X │ X │ X │ X │
	* ├────────────────┼──────────┼─────────┼─────────┼─────────┼─────────┤
	* │Optimized │ │ │ │ X │ X │
	* ├────────────────┼──────────┼─────────┼─────────┼─────────┼─────────┤
	* │Minimal │ │ │ │ │ X │
	* ├────────────────┼──────────┼─────────┼─────────┼─────────┼─────────┤
	* │None (N/A yet) │ │ │ │ │ │
	* └────────────────┴──────────┴─────────┴─────────┴─────────┴─────────┘
	* </pre>
	*
	* <p>Please note that, in development mode (jre.checkedMode=ENABLED), these checks will always be
	* performed regardless of configuration but will be converted to AssertionError if check is
	* disabled. This so that any reliance on related exceptions could be detected early on.
	* For this detection to work properly; it is important for apps to share the same config in
	* all environments.
	*/
	// Some parts adapted from Guava
	public final class InternalPreconditions {

Getting distracted from the fix itself, you mentioned compression - are you compressing your JS (GWT can do it at compile time if you don't mind gzip, and your server likely can do brotli), and are you doing other things that will decrease output size like compiler.stackMode=strip (allows deduplicating some functions where their contents are the same)?

[vjay82]

I also think, this is a cleaner approach.

Distraction: Thank you for the hint. Yes, setting that too, just forgot to mention it, as it is hidden away in the module.xml (and not in the build script itself). We are using native stack traces and utilizing ZStd, brotli or gzip. Also we remove some strings from lambda clinit (which you are probably addressing more efficiently very soon when looking at the issues-tab) in a post-compile-step.

There are other low-hanging fruits in the output of the iframe linker (lots of escaped line-breaks) but removing those messes up stack traces of course so not doing that. I asked myself, why things have to go through eval in the first place (nowadays).

Also thanks for the continued development.

[niloc132]

I'm doing a little more exploring in that area now - if you have other observations that seem appropriate to files as bugs, I'd welcome that.

I went digging into the newline thing - there are some legacy reasons why certain output decisions had to be made (limit identifiers in a var for an old safari bug, etc), but it looks like the newline thing doesn't exist for a particular reason. I found #986 discussing this and the matching commit e5fbf1c that purports to fix it, yet always adds newlines after function statements (where the } should be sufficient always). With that said, 442cf33 discusses how sourcemap size might be impacted by such a change, so we might want to let this be configurable.

Clinit removal works ... except for super dev mode, so it needs more work. This other works is related only in that I got irritated at seeing yet another empty clinit decl and double-clinit call and wanted to chase them down, see if other improvements could pop out too.

Regarding indexOf, there are two small improvements I have from looking at the apps I have readily available to me:

• JS String.includes is a thing in all browsers now, which at least saves bytes in the s.indexOf(h)!=-1 case
• Java String.indexOf(char) with a literal char optimizes pretty badly, since GWT will have to first turn the literal char into an int, the int into a string via JS String.fromCharCode - if we statically know it is a char to begin with, we could just compile it to a String. For example, java s.indexOf('.') is the same as js s.indexOf('.')) such as in

  gwt/user/super/com/google/gwt/emul/java/util/logging/LogManager.java

  Line 72 in 7c36c89

  String parentName = name.substring(0, Math.max(0, name.lastIndexOf('.')));

  In order to make this actually work though, String.valueOf(char) cannot be inlined until much later (if ever), since otherwise the inlining can end up happening before the static eval from char to string happens. On the plus side, this more or less prevents another occurrence of your indexOf issue above.

Okay, probably enough distraction here. File issues you'd like to see get worked on, and we can collaborate as time/effort permits to make them happen.