[wip] introduce option to consider scala companion objects stable

[fwbrasil]

Work in progress

Code is functional, but still lacks unit tests.

Problem

In order to support Java 9, the Scala compiler developers had to make the companion object fields not final in Scala 2.12. See scala/scala-dev#194 and scala/scala3#1441.

This new encoding doesn't allow Graal to perform constant folding of companion objects and its final fields.

Solution

Introduce the option ConsiderScalaCompanionObjectsStable that enables an heuristic that determines if a field is a scala companion object and considers it as a stable value.

Notes

1. Would you be comfortable with introducing this kind of language-specific heuristic?

2. Is it ok to use the system class loader here?

[graalvmbot]

• Hello Flavio Brasil, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address fbrasil -(at)- twitter -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

[fwbrasil]

Twitter has already signed the OCA, but I've also submitted the form by email.

[graalvmbot]

• Flavio Brasil has signed the Oracle Contributor Agreement (based on email address fbrasil -(at)- twitter -(dot)- com) so can contribute to this repository.

[dougxc]

I can understand the motivation for such a change. However, we've avoided this kind of thing so far as it is basically adding intrinsics for something that Graal cannot prove came from a trusted source (unlike intrinsics for JDK classes). That said, putting it behind an option (whose default is false) mitigates that concern to some degree. What do you think @thomaswue ?

BTW, I know it's only available as of JDK 11, but maybe Dynamic Constants can solve this scala use case?

[dougxc]

I don't think this code is correct. You really want the class loader of declaringClass. This class loader is not exposed by JVMCI.

[fwbrasil]

How can I get a hold of the correct class loader? Or is it not possible? I used the system class loader because it seems that's how other parts of the code base load classes

[dougxc]

Class loaders are intentionally not part of JVMCI as JVMCI must also operate in environments where class loaders may not be supported (e.g. SVM and libgraal). What you want is the API introduced recently to resolve a String to a JavaType. Note however that this API is HotSpot specific.

[fwbrasil]

@dougxc according to @retronym, we could avoid loading the class here if we read the Scala attribute from declaringClass. Is there a way to access class atributes? I couldn't find anything looking at ResolvedJavaType

[thomaswue]

Maybe it is possible to keep the core mechanism language-agnostic and move the Scala-specific part into the configuration (i.e., flag value or flag configuration file)?

[fwbrasil]

@dougxc @thomaswue thank you for the reviews! I commented on the Scala issue mentioning the new Dynamic Variable feature.

@thomaswue is your suggestion a more generic flag indicating that the application is in Scala? The optimization is already under a flag and disabled by default (ConsiderScalaCompanionObjectsStable)

[dougxc]

Yes, if we put this behind a flag, it has to be something generic that doesn't imply it is specific to Scala. I was thinking of something like -Dgraal.CompilationFinalFields=<regex>. However, that doesn't help in the Scala case because of the need to establish the relationship between companion classes - a pattern matching all fields named MODULE$ would be too general. This suggests dynamically loaded compiler extensions which in turn require a well defined and stable extension mechanism.

[retronym]

This new encoding doesn't allow Graal to perform constant folding of companion objects and its final fields.

The old encoding, with a static final field that was assigned in <init>, rather than <clinit> was also not inlinable (at least by C2 in Java 9+) because the JVM detects the disallowed assignment and treats the field as non-final for JIT purposes.

Our language semantics allow the uninitialized module field to be witnessed as null if it is referenced circularly during the call to the constructor of the superclass of the module. So I don't think that condy is suitable.

The @Stable annotation (where it available outside the JDK itself), are possible solutions. Could Graal provide an option to mark fields as @Stable by regex?

The @Stable annotation can be simulated with invokedynamic, where a bootstrap method collapses into a ConstantCallSite once field becomes non-null.

I have a slight concern in using this compilation technique though because I've noticed that JDK 8 (which we still support) + C2 leaves some overhead, even after inlined ConstantCallSite-s. I think https://github.com/Project-Skara/jdk/commit/aec826cfdaba559d51d2895b191dd4896b9d393a removes some/all of it in 9+.

[dougxc]

As of JDK 9, there is the jdk.internal.vm.annotation.Stable annotation. Graal recognizes this annotation. However, since this type is only exported to select modules, using it will require something like --add-exports=java.base/jdk.internal.vm.annotation=ALL-UNNAMED on the java command line.

[fwbrasil]

The old encoding, with a static final field that was assigned in , rather than was also not inlinable (at least by C2 in Java 9+) because the JVM detects the disallowed assignment and treats the field as non-final for JIT purposes.

@retronym that's not the case for Graal, it does constant folding of companion objects in Scala 2.11

[fwbrasil]

Our language semantics allow the uninitialized module field to be witnessed as null if it is referenced circularly during the call to the constructor of the superclass of the module. So I don't think that condy is suitable.

@retronym Could you clarify this statement? Do you mean that the heuristic is not enough? I imagine the field being witnessed as null wouldn't happen at the phase that the JIT kicks in.

@dougxc @thomaswue this seems to have a considerable performance impact. I share of your concerns that this kind of rule is not ideal, but I wonder if this issue could affect the Graal adoption by the Scala community. We've been advocating Graal as an alternative to improve the performance of Scala applications, but if people don't see improvements using Scala 2.12 it's unlikely they'll adopt Graal. Note that Twitter is still on Scala 2.11, which is an exception. Most companies are already on Scala 2.12.

[dougxc]

I think an acceptable solution might be to define a StableFieldProvider service in Graal:

    public abstract class StableFieldProvider {
        public interface Tool {
            ResolvedJavaType lookupType(String name, ResolvedJavaType accessingType) throws ClassNotFoundException;
        }
        protected abstract boolean isStable(ResolvedJavaField field, Tool tool);
    }

and provide a Scala specific implementation:

    public class ScalaStableFieldProvider extends StableFieldProvider {
        @Override
        protected boolean isStable(ResolvedJavaField field, Tool tool) {
            if (field.isStatic()) {
                ResolvedJavaType declaringClass = field.getDeclaringClass();
                if (declaringClass.isInitialized() && field.getName().endsWith("MODULE$")) {
                    String declaringClassName = declaringClass.toClassName();
                    if (declaringClassName.endsWith("$")) {
                        String companionInstanceClassName = declaringClassName.substring(0, declaringClassName.length() - 1);
                        try {
                            ResolvedJavaType companionInstanceClass = tool.lookupType(companionInstanceClassName, declaringClass);
                            return isScalaClass(companionInstanceClass);
                        } catch (ClassNotFoundException e) {
                            return false;
                        }
                    }
                }
            }
            return false;
        }

        private static boolean isScalaClass(ResolvedJavaType cls) {
            for (Annotation annotation : cls.getAnnotations()) {
                if (annotation.annotationType().getName().equals("scala.reflect.ScalaSignature")) {
                    return true;
                }
            }
            return (cls.getEnclosingType() != null) ? isScalaClass(cls.getEnclosingType()) : false;
        }
    }

It means that instead of a Graal option, you would have to package ScalaStableFieldProvider deployment as a service provider. It will also require extra command line options for deploying the service. On JDK8, it will be something like:

-Djvmci.class.path.append=path/to/scala-stable-field-provider.jar

and on JDK9+:

--module-path=path/to/scala-stable-field-provider.jar --add-exports=jdk.internal.vm.ci/jdk.vm.ci.meta=scala-stable-field-provider --add-exports=jdk.internal.vm.compiler/org.graalvm.compiler.core.common.spi=scala-stable-field-provider

The --add-exports options are required since JVMCI and Graal are not exported by default.

[retronym]

@dougxc That sounds like a good solution.

The simplest and correct way to check for isScalaClass is to look for the marker attribute "Scala". Scala objects need not have a companion class, so your check above would fail for object D.

class C
object C

object D

$ jardiff -raw '/tmp/C$.class'

+++ C$.class.asm
// class version 52.0 (52)
// access flags 0x31
public final class C$ {

  // compiled from: test.scala

  ATTRIBUTE Scala : unknown

$ jardiff -raw '/tmp/D$.class'

+++ D$.class.asm
// class version 52.0 (52)
// access flags 0x31
public final class D$ {

  // compiled from: test.scala

  ATTRIBUTE Scala : unknown
...

@fwbrasil Here's a test case that shows that a call site could witness null and then later witness the initialized field.

The field could also change if someone reflectively calls O.<init> later on.

// class C { println(O) }
// object O extends C { println("O.init")}

class C {
    C() {
        Test.useO();
    }
}
class O extends C {
    /*stable*/ static O instance; // In Scala 2.11, this was also ACC_FINAL
    static {
         new O();
    }
    private O() {
        super();
        instance = this;
        Test.useO();
    }
}

public class Test {
    static void useO() {
       System.out.println(O.instance); // JIT shouldn't cache the null here
    }
    public static void main(String[] args) {
         O o = O.instance;
    }
}

[retronym]

I suppose another solution, based on existing extension points, would be to use a javaagent to add the @Stable annotation to the MODULE$ fields.

On JDK9+, this would likely require -add-exports=java.base/jdk.internal.vm.annotation=ALL-UNNAMED. On 8, the application classes would need to be placed in the boot classloader, which isn't ideal.

[dougxc]

The javaagent suggestion is interesting although I assume it will impose quite a cost to startup (in addition to the problem of having to put app classes on the boot class path).

With respect to your useO example, the System.out.println(O.instance) call should not see null since accessing the static O.instance field will trigger O.<clinit>.

[dougxc]

After speaking with @vjovanov and @axel22 , I can see why having to put extra things on a Scala command line would be a barrier for most Scala users. However, this has to be balanced against the risk of enabling a compiler optimization that can cause confusing behavior (e.g. someone uses reflection to re-assign a MODULE$ field after it has been compiled as constant by Graal). By requiring extra command line options, you are getting an explicit acknowledgement from the user that they're aware of the risk and willing to accept it for better performance.

Speaking of performance, how much performance are we talking about? Are there benchmarks you can discuss? Would this optimization noticeably improve the performance of scalac for example?

[dougxc]

One general comment is that its a shame that there is no general purpose CompilerPragma annotation in the JDK that could be used for cases like this.

[retronym]

should not see null

In the example, that method is called from within <clinit> before the field is assigned. It's a corner case, of course.

Are there benchmarks you can discuss?

Here's some benchmarking work:

rorygraves/scalac_perf#32 (comment)

Partially reproducing here:

class MathTest {
  @Benchmark def new2Min(bh: Blackhole) = newMath2.module.min(bh.i1, bh.i2)
  @Benchmark def new3Min(bh: Blackhole) = newMath3.module.min(bh.i1, bh.i2)
}

final class newMath2 {
    static newMath2 module;
    static {
        module = new newMath2();
    }
    int min (int i1, int i2) { return  Math.min(i1, i2);}
}
final class newMath3 {
    final static newMath3 module;
    static {
        module = new newMath3();
    }
    int min (int i1, int i2) { return  Math.min(i1, i2);}
}

[info] Benchmark                    Mode  Cnt          Score          Error  Units
[info] m.scala.MathTest.new2Min    thrpt   10  232759741.736 ´�¢ 44329349.640  ops/s
[info] m.scala.MathTest.new3Min    thrpt   10  267180137.855 ´�¢ 18586316.540  ops/s

That benchmark shows the cost of wrapping java.lang.Math.min in a forwarder in the Scala object scala.math.package$.min is about 14%. Presumably, this is because JIT can't eliminate the field access and null check before getting to the inlined min implementation.

Would this optimization noticeably improve the performance of scalac for example?

I think we'd need to try it out to see. I've been meaning to do this with @Stable, but haven't gotten around to it yet. My gut feel is "probably not", but I'm also used to being surprised by performance!

[axel22]

The difference between the two versions is only in the null-check guard.

I do agree that the microbenchmark itself is already a pain, since it makes it hard to write performance-critical algorithms in Scala. The only current way around it is to encode program constants as Scala macros (which is not a too bad solution for people that measure and care about performance, IMO).

Otherwise, I think that the best solution would be to have a field annotation that allows saying @hint("stable") that tells a JIT compiler that it's safe to assume that the field will never change (and which could even be enforced through bytecode verification). Sadly, there's nothing like that in the JDK right now - perhaps it would make sense to bring this up with the JPG people.

Without the annotation, the stable field provider approach suggested by Doug seems like the best solution to me.

[fwbrasil]

I observe similar performance overhead in more complex scenarios. For instance:

  val mapF: Int => Int = _ + 1

  @Benchmark
  def map = Promise[Int]().map(mapF).map(mapF).map(mapF)

# with ConsiderScalaCompanionObjectsStable=false
FutureBench.map thrpt   10  8857268.932 ± 1254596.248   ops/s

# with ConsiderScalaCompanionObjectsStable=true
FutureBench.map thrpt   10  9707714.600 ± 1001811.164   ops/s

# about 9% performance overhead

I actually identified this issue while testing #636. The CAS virtualization can happen only if the offset is a constant. In Scala 2.11 it is, but in Scala 2.12 it's not since the constant folding can't fold the final offset field in the Promise companion object.

[dougxc]

@fwbrasil , if the StableFieldProvider proposal sounds good to you, do you want to flesh it out in this PR? Alternatively, I can create a new PR.

[axel22]

Quick question. @gilles-duboscq and I were looking at some companion object code, and we noticed bytecode like this getting emitted:

object Foo

  public static {};
    descriptor: ()V
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=1, locals=0, args_size=0
         0: new           #2                  // class Foo$
         3: invokespecial #22                 // Method "<init>":()V
         6: return
...
  private Foo$();
    descriptor: ()V
    flags: ACC_PRIVATE
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokespecial #53                 // Method java/lang/Object."<init>":()V
         4: aload_0
         5: putstatic     #55                 // Field MODULE$:LFoo$;
         8: return

Why not publish the MODULE$ field earlier in the <clinit> instead. In other words do:

  public static {};
    descriptor: ()V
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=1, locals=0, args_size=0
         0: new           #2                  // class Foo$
         3: dup
         4: invokespecial #22                 // Method "<init>":()V
         7: putstatic     #55                 // Field MODULE$:LFoo$;
         10: return

or, if NPE are a problem, to publish the MODULE$ field before calling the constructor:

  public static {};
    descriptor: ()V
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=1, locals=0, args_size=0
         0: new           #2                  // class Foo$
         3: dup
         4: putstatic     #55                 // Field MODULE$:LFoo$;
         7: invokespecial #22                 // Method "<init>":()V
         10: return

If you do either, it seems like the MODULE$ field could be made final.

[axel22]

Ok, we just saw Jason's comment in scala/scala3#1441 :

current behavior publishes a partially initialized object that other classes can use(see the example above). If we prohibit this, it would mean that in the body of the object you cannot use anything that transitively uses the same object. I believe this is a very harsh restriction that is hard to maintain in big codebases.

class A {
  def foo = O.bar
}

object O extends A {
  foo
  def bar = println("bar")
}

This emits this code:

  public static {};
    descriptor: ()V
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=1, locals=0, args_size=0
         0: new           #2                  // class O$
         3: invokespecial #12                 // Method "<init>":()V
         6: return
...
  private O$();
    descriptor: ()V
    flags: ACC_PRIVATE
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokespecial #25                 // Method A."<init>":()V
         4: aload_0
         5: putstatic     #27                 // Field MODULE$:LO$;
         8: aload_0
         9: invokevirtual #30                 // Method foo:()V
        12: return

Could scalac instead emit the "super" part of the ctor in the static initializer, and publish the MODULE$ field there, and then continue the initialization in the companion object constructor:

  public static {};
    descriptor: ()V
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=1, locals=0, args_size=0
         0: new           #2                  // class O$
         3: dup
         4: invokespecial #25                 // Method A."<init>":()V
         7: dup
         10: putstatic     #27                 // Field MODULE$:LO$;
         13: invokespecial #12                 // Method "<init>":()V
         16: return
...
  private O$();
    descriptor: ()V
    flags: ACC_PRIVATE
    Code:
      stack=1, locals=1, args_size=1
         8: aload_0
         9: invokevirtual #30                 // Method foo:()V
        12: return

[fwbrasil]

@fwbrasil , if the StableFieldProvider proposal sounds good to you, do you want to flesh it out in this PR? Alternatively, I can create a new PR.

@dougxc I can work on it if we agree it's a good solution. I'm working on other scala-specific optimizations, so I wonder if we should create a separate project to bundle them and have releases for each Scala major version. Or should it be a Graal module? wdyt @christhalinger?

[christhalinger]

I think the service provider proposal is a good technical solution but it might not be exactly what we, and especially the Oracle Labs folk, want. Twitter is advertising and advocating for using Scala with Graal for a while now because it works really well out-of-the-box. It would be a shame if that advantage is gone with Scala 2.12. And we all know that the first impression sticks for a very long time.

A solution where you have to add a bunch of command line options and an additional module is not very user friendly. For us at Twitter it doesn't matter; we know what Graal can do for us and we wouldn't have a problem adding the additional options.

But we should think about all the other Scala users we would like to use Graal...

[dougxc]

I'm with you @christhalinger - Graal should be the preferred choice for running Scala. That said, the solution originally proposed in this PR already requires an explicit command line option. We cannot allow Graal to deviate from Java semantics by default without some opt-in by the user (e.g., on the command line). And as you say, in environments where performance matters, extra command line options will not be a barrier.

Maybe there is some other way to really solve this with pure Java? Not being a Scala language expert (or user!), I cannot assess how large the pure-Java solution space is or whether it has been fully exhausted. Maybe there are experts at Twitter or watching this thread who can chime in.

[dougxc]

BTW, the proposal for rewriting the initialization sequence will almost certainly not verify (you cannot directly call a super constructor on the result of a new bytecode).

[christhalinger]

Yes, I absolutely agree with the "deviate from Java semantics" point. What do you mean by "pure Java" solution? Do you mean a change in the Scala language implementation? Also not a Scala expert (or user!) here ;-) Embarrassing...

[dougxc]

Yes, I'm hoping a change in the Scala language implementation might be possible. Something along the lines of what @axel22 proposed above but something that will also pass the bytecode verifier ;-) @gilles-duboscq had one possible idea but is not sure about its broader implications. Can you please outline that idea here Gilles.

[retronym]

We have added a special case to the compiler to defer assignment to the MODULE$ field until <clinit> when the object has an empty constructor.

For instance:

object O { def min(x: Int, y: Int) = java.lang.Math.min(x, y) }

Is translated to:

+++ O$.class.asm
// class version 52.0 (52)
// access flags 0x31
public final class O$ {


  // access flags 0x19
  public final static LO$; MODULE$

  // access flags 0x9
  public static <clinit>()V
    NEW O$
    DUP
    INVOKESPECIAL O$.<init> ()V
    PUTSTATIC O$.MODULE$ : LO$;
    RETURN
    MAXSTACK = 2
    MAXLOCALS = 0

  // access flags 0x2
  private <init>()V
    ALOAD 0
    INVOKESPECIAL java/lang/Object.<init> ()V
    RETURN
    MAXSTACK = 1
    MAXLOCALS = 1

  // access flags 0x1
  public min(II)I
    // parameter final  x
    // parameter final  y
    ILOAD 1
    ILOAD 2
    INVOKESTATIC java/lang/Math.min (II)I
    IRETURN
    MAXSTACK = 2
    MAXLOCALS = 3
}

We can extend this special case to object that don't assign to their (ACC_FINAL), the constructor body (other than the super call) could be moved out to <clinit>.

I'm also pursuing the more general invokedynamic approach in scala/scala#7195. I'm using the compiler itself as a benchmark, it seems to be on par with old warmed up performance under 1.8 C2. We'll need to evaluate cold performance, which I'd expect to be worse, as well as evaluate a microbenchmark to show that the invokedynamic / ConstantCallSite doesn't leave any residual overhead.

[axel22]

@retronym 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 <clinit>.

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]

That sounds like it would work, and would avoid the need to break out to Java to define static final constants for inlinable MethodHandles, etc.

We'll have to get there gradually; it's too late to consider this change (which risks breaking reflective apps) or the invokedynamic solution (which seems to hurt cold performance by 25%) for 2.13.0. Let's continue the Scala compiler discussion in scala/scala-dev#537.

In the interim, having a heuristic in Graal to deal with the current encoding would be welcome and worthwhile.

[dougxc]

We will continue to try improve Scala performance in general. Given that there now appears to be a scalac-based solution for good performance of Scala companion objects/classes, there's no need for the Scala specific switch proposed in this PR. Thanks for the useful discussion and feel free to suggest other Graal compiler heuristics that may be useful for Scala.

[fwbrasil]

@dougxc I'm not sure I understand. As @retronym mentioned, the language change might eventually happen, but it won't be implemented for the current major release (2.12), and won't make it to the next major release (2.13). Should we recommend people to avoid Graal for these releases? It's likely that companies will take years to be on a release > 2.13

[dougxc]

Graal already has performance advantages for executing Scala without this specific change so I'm not sure it makes sense to recommend that Scala users avoid Graal.

[christhalinger]

I agree with @fwbrasil and I think it's a mistake to close this bug. At VMM I talked with @thomaswue about this issue and it seemed he was very interested in solving this issue for the current Scala version.

[dougxc]

Hey @christhalinger , we're all on the same page with respect to wanting Scala companion objects to be fast on Graal. However, this specific PR contains a proposal to do it in a way that deviates from Java semantics based on a Scala specific option.

If you have other proposals that don't have these drawbacks, I'd love to hear them ;-)

[christhalinger]

Working on it... ;-)