Eliminating <clinit> on Global objects

[realityforge]

Currently the global object created for each module ends up potentially having a <clinit> if there is variables accessed in scope. For example elemental2.dom.DomGlobal has a elemental2.dom.DomGlobal__Constants. The DomGlobal__Constants class has static fields mapped to javascript and then DomGlobal re-esposes those fields by using a static final field. This results in DomGlobal having a <clinit> method that simply assigns the DomGlobal__Constants fields to the DomGlobal fields. It also mandates that most code that accesses these fields must call the clinit method first.

The whole <clinit> dance seems like unnecessary overhead and I am trying to understand why the fields are not moved directly to the DomGlobal class. It would result in less code and have practically the same developer experience - the only difference that I am aware is that it does not stop user code re-assigning the variables as can not be final.

Is there any other reason for this approach?

[niloc132]

My guess is the use of the final keyword is at issue here - note the use of final only on those JsOverlay fields, the others like DomGlobal.console and customElements and such are all non-final. Technically that means you could reassign them from in user code (but of course the browser ought to be smart enough to disallow this).

Looking at window_patched.js output from the elemental2 build, I want to guess that this is just the @const-annotated elements. From that, it looks like jsinterop-generator is trying to ensure that we can't possibly reassign those fields (though from that perspective, many more of these ought to be tagged in the same way).

For a workaround you probably could just disable that const check, or remove all of those @const annotations? I'm not sure a compiler could work out that aliasing and just use the non-overlay field directly, at least from your digging it doesn't appear that GWT2 can, might be worth double checking that closure is able to (since j2cl emits a roughly equivalent clinit the last time I checked, though admittedly I wasn't digging into jstype output).

[realityforge]

The reason is roughly that all native static fields on types go through this __Constants generation. See https://github.com/realityforge/jsinterop-generator/blob/7dc62bb37fa0716f8b7fdef13798e094a4f8c818/java/jsinterop/generator/visitor/FieldsConverter.java#L125

If my assumptions above are right and we are willing to let the fields be non-final when reflected into java then the <clinit> can be removed via

diff --git a/java/jsinterop/generator/visitor/FieldsConverter.java b/java/jsinterop/generator/visitor/FieldsConverter.java
index 67b32e6..06bf856 100644
--- a/java/jsinterop/generator/visitor/FieldsConverter.java
+++ b/java/jsinterop/generator/visitor/FieldsConverter.java
@@ -130,6 +130,9 @@ public class FieldsConverter extends AbstractModelVisitor {
           originalType.getFields().stream().noneMatch(f -> f.isStatic() && !f.isNativeReadOnly()),
           "Non constant static fields are not supported on interface");
     }
+    if (originalType.isClass() || originalType.isNamespace()) {
+      return;
+    }
 
     Type constantWrapper = new Type(EntityKind.CLASS);
     constantWrapper.setAccessModifier(DEFAULT);

This seems to work with preliminary tests but before I went further and actually verified it across everything and tried to get it merged I just wanted to get some feedback. I guess I can try it out locally and send in the patch regardless. May try tonight

[gkdn]

From the relevant design doc:

The current implementation has some drawbacks:

• We don’t support static fields on interfaces because static fields are converted to a getter and interface doesn’t support native static methods.
• Users can potentially write code that assign a value to a constant field.

So this current __Constants solution address both of the issues. This doesn't have any impact on J2CL where this is optimized away but we didn't invest on GWT2 to write an optimization for it.

[realityforge]

Would you consider a pull request that added an option like --use_constants_overlay_for_interfaces_only that removed the __Constants class unless it was needed to support static fields for an interface. It would default to current behaviour but when building artifacts for GWT2.x I could configure it to use a more optimal path.

It would still allow users to potentially write code that assigned a value to these fields but this would be blocked by the javascript runtime so it seems like less of an issue.

[gkdn]

I'm not completely against it but maybe somebody could contribute a GWT compiler optimization instead?

[realityforge]

That would certainly be a better solution but I don't currently have the cycles that I would need to get that right. (Or the inclination as we want to move to j2cl as soon as possible). I will try try to put together a pull request for the above when I get the time.

[realityforge]

After experimentation, we have decided to move directly to J2CL so no longer see this as a priority. So I will close the issue until plans change.

[gkdn]

I'll re-open this since I noticed we have some patterns that doesn't optimize in J2CL. Either we should improve optimizations or change the implementation.