Make MODULE$ Final Again?

[retronym]

We had to (#194) make the module field non-final to prepare for JDK9/Classfile 53 verifier, which enforces the (long standing) rule that static final field assignments must occur in <clinit> (not <init>).

However, this prevents the JIT from eliding the access of this field in calling code. For example, in scala.math.max(1, 2), the JIT can't rule out that scala.math.package$.MODULE$ hasn't been nulled out, so it must be checked.

For some special cases, like above, I've managed to restore finality of that field in scala/scala#6523. But important modules don't qualify for the optimization, e.g Predef and Nil.

The JDK has an internal annotation, @Stable, that would tell the JIT that this field is 'effectively final', ie it is assign once. That's not available for our purposes. It can be simulated with invokedynamic, which might be worth trying, but we need to ensure it is really zero-overhead across different JDK versions.

Another idea came to mind yesterday.

Given:

trait T {
  println("T")
}
class C {
  println("C")
}
object O extends C with T {
  println("body")
}

We currently emit:

public final class O$ extends C  implements T  {

  public static LO$; MODULE$

  public static <clinit>()V
    NEW O$
    INVOKESPECIAL O$.<init> ()V
    RETURN

  private <init>()V
    ALOAD 0
    INVOKESPECIAL C.<init> ()V
    ALOAD 0
    PUTSTATIC O$.MODULE$ : LO$;
    ALOAD 0
    INVOKESTATIC T.$init$ (LT;)V (itf)
    GETSTATIC scala/Predef$.MODULE$ : Lscala/Predef$;
    LDC "body"
    INVOKEVIRTUAL scala/Predef$.println (Ljava/lang/Object;)V
    RETURN
}

But why not:

public final class O$ extends C  implements T  {

  public static final LO$; MODULE$

  public static <clinit>()V
    NEW O$
    DUP
    INVOKESPECIAL O$.<init> ()V
    DUP
    PUTSTATIC O$.MODULE$ : LO$;
    INVOKESPECIAL O$.$init$
    RETURN

  private <init>()V
    ALOAD 0
    INVOKESPECIAL C.<init> ()V

  private $init$()V
    ALOAD 0
    INVOKESTATIC T.$init$ (LT;)V (itf)
    GETSTATIC scala/Predef$.MODULE$ : Lscala/Predef$;
    LDC "body"
    INVOKEVIRTUAL scala/Predef$.println (Ljava/lang/Object;)V

}

ie, split the constructor up just after the super-class constructor call and move the rest into a private method that would be called after <clinit> assigns the module field.

[retronym]

@lrytz @adriaanm WDYT?

One incompatibility is that reflectively instantiating the module's private constructor (either nefariously or accidentally with help from a misconfigured framework like kryo) would no longer get a fully initialized instance.

[retronym]

/cc @mkeskells @rorygraves

[odersky]

If we put the actual module initialization in a private method, would that not break the definite assignment rules for final fields in the object? I believe that's the case for Java the language, but I am not sure to what degree the JVM enforces it.

[retronym]

@odersky You're right. It was too good to be true!

The update to the verifier in question here, that prevents static final assignment outside of <clinit>, also enforces the rule that non-static final fields can only be assigned in <init>. In fact, we're about to change the trait encoding to make mixin final vals non-final in bytecode (we'll add an explicit memory barrier to fill in for the implicit one that the JVM would have inserted thanks to the final field).

It is worth noting the class-file version dependent treatment of non-conformant classfiles in JDK9+:

• If the classfile version is 53 or higher, its a verify error
• otherwise, JIT disables the FoldStableValues optimizations anyway, ignoring the final modifier.

@mkeskells showed the effect that JIT optimizations can make: rorygraves/scalac_perf#32 (comment)

That suggests scala.math.min seems to be 17% slower than the underlying Math.min that is forwards to, which can be recovered with a final module field.

[viktorklang]

@retronym Is there a difference CONDY would make here?

[retronym]

It isn't exactly a constant because the null value can be observed from the superclass.

class C {
  println("O = " + O)
}

object O extends C

object Main extends App {
  O.toString
  new C
}

$ scalac /tmp/test.scala && scala Main
O = null
O = O$@7d6f77cc

We could, however, use InvokeDynamic to intermediate the access to the module field with a bootstrap method that collapses into a ConstantCallSite as soon as the first non-null value of the field is witnessed. This idea is implemented in a general purpose way in https://github.com/forax/exotic#stablefield---javadoc.

That's worth trying, but does require some care as we'd be using the technique very widely and we need a high degree of certainty that it behaves and performs as we'd like in all environments.

[viktorklang]

@retronym I'd be extremely interested in hearing about how INDY usage for this would work out. If you do pursue it, please let me know.

[mkeskells]

@viktorklang FYI - I had a couple of experements with Reme exotics library https://github.com/forax/exotic and the performance is very different in different VMs (tested JDK 8,9,10). Depends on the technique though

[fwbrasil]

FIY the fact that the modules are not final anymore also breaks constant folding in Graal (see oracle/graal#637). @dougxc mentioned that in Java 11 Dynamic Constants might be an alternative to solve this problem.

[retronym]

From oracle/graal#637 (comment)

Here is a short summary of the solution that @gilles-duboscq came up with - objects that assign to their ACC_FINAL could instead put that state into static fields. That way, any singleton constructor could be emitted inside the .

The singleton object fields (whether ACC_FINAL or not) are anyway accessed through accessors, so it seems like the entire singleton object state be kept in static fields. Serialization also already makes sure that the singleton object does not get duplicated.
Other than various reflection-reliant libraries, what else could such a change affect?

[retronym]

This seems like a good solution to me. The bonus benefit is that fields of objects would themselves be constant foldable, without relying on -XX:+TrustNonStaticFinalFields. A canonical use case would be declaring static final MethodHandle constants.

We're too late in the Scala 2.13 cycle to introduce this encoding. But it would be great for the next version.

[smarter]

What does "assign to their ACC_FINAL" mean ? Could you sketch what the bytecode would look like with this proposal ?

[retronym]

  object ModuleStatic extends C() with T {
    val x = 42
    System.out.println("ModuleStatic")
    var y = ""
  }
  class C; trait T { System.out.println("T") }

public final class ModuleStatic$ extends C implements T {
  public static final ModuleStatic$ MODULE$;
  private static final int x;
  private static String y;

  private ModuleStatic$() {
        super();
  }
  static {
    MODULE$ = new ModuleStatic$();
    T.$init$(MODULE$);
    x = 42;
    System.out.println("ModuleStatic");
    y = "";
  }
  public int x() {
    return x;
  }
  public String y() {
    return y;
  }
  public void y_$eq(String value) {
    y = value;
  }
}
class C {
  
}
interface T {
  static void $init$(T t) {
    System.out.println("T");
  }
}

[odersky]

That looks like an excellent solution! I wonder why we did not think before about making all object fields static. - Martin

…

On Wed, Sep 12, 2018 at 3:04 PM Jason Zaugg ***@***.***> wrote: object ModuleStatic extends C() with T { val x = 42 System.out.println("ModuleStatic") val y = "" } class C; trait T { System.out.println("T") } public class ModuleStatic$ extends C implements T { public static final ModuleStatic$ MODULE$; private static final int x; private static String y; private ModuleStatic$() { super(); } static { MODULE$ = new ModuleStatic$(); T.$init$(MODULE$); x = 42; System.out.println("ModuleStatic"); y = ""; } public int x() { return x; } public String y() { return y; } public void y_$eq(String value) { y = value; } }class C { }interface T { static void $init$(T t) { System.out.println("T"); } } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#537 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAwlVhI-qbgvueX7-nAaDcnr6Q641c6Nks5uaQWNgaJpZM4V7t7y> .

-- Martin Odersky EPFL and Lightbend

[viktorklang]

Are there any risks w.r.t. partial initialization? (i.e. if there's an exception somewhere after starting to initialize fields.)

[retronym]

@viktorklang I think the situation is the same as before. An exception thrown by the constructor of the object still bubble out of <clinit> and render the class as erroroneous. Subsequent access would result in a NoClassDefFoundError.

The stack trace of the exception would start at <clinit>, rather than <init> :: <clinit> as before.

[retronym]

The one behaviour that changes is what happens when someone reflectively calls the private constructor.

The status quo is already broken: the MODULE$ field is re-assigned. As written above, the instance field would not be reassigned, but we would leak out a second instance of the singleton to the reflective caller.

If we're changing things, I'd suggest that we assert that MODULE$ is null in the constructor to turn this into a hard and fast failure.

[retronym]

Another nice property about this encoding is that it doesn't necessarily break reflective access to fields.

scala> val m = ModuleStatic$.MODULE$
T
ModuleStatic
m: ModuleStatic$ = ModuleStatic$@10f7c76

scala> val x = m.getClass.getDeclaredField("x")
x: java.lang.reflect.Field = private static final int ModuleStatic$.x

scala> x.setAccessible(true)

scala> x.get(m) // No need to pass an instance anymore, but legacy code that does won't break
res2: Object = 42

scala> x.get(null)
res3: Object = 42

[mkeskells]

This is what we discussed in rorygraves/scalac_perf#19 such a long time ago. Strange how these things go in cycles.

Are we really too late in 2.13 for this?
We still have planned binary incompatible changes to make

This change would give us faster execution, and (if we can use the static fields directly in bytecode) smaller classfiles as well for some access patterns, including the common ones.

Should we not consider the benefit with a small benchmark before dismissing the option?

[viktorklang]

@retronym Good, thanks for elaborating on that.

[adriaanm]

I can't commit to including this change in what is primarily a library release, with the dev cycle in feature freeze, but I'm open to being convinced by substantial gains in relevant benchmarks and due diligence in avoiding regressions. I'd like to make a decision by the end of October, which means we need to have a reviewable PR with a passing community build and benchmarks by then.