Make object fields static, and move post-super init to <clinit> #7270
Conversation
|
TODO:
|
| if (isCZStaticModule || isCZParcelable) { | ||
| fabricateStaticInit() | ||
| // TODO should we do this transformation earlier, say in Constructors? Or would that just cause | ||
| // pain for scala-{js, native}? |
There was a problem hiding this comment.
I haven't made my mind about it yet. I'll think about it.
Can my input wait until next week? I'm currently at the Scala Symposium.
retronym
force-pushed
the
topic/static-object-fields
branch
from
ae095f1 to
815e210
Compare
|
There is one interesting failure in Minimized: trait CustomExecutionContext {
val inEC = ""
def testOnSuccessCustomEC(): Unit = {
val t = new Thread() {
override def run(): Unit = {
println("getting it")
inEC.length
// this.outer$.inEC() now blocks until the `Test$.<clinit>` has concluded, because `Test$.inEC()`
// now differs:
// public inEC()Ljava/lang/String;
// - ALOAD 0
// - GETFIELD Test$.inEC : Ljava/lang/String;
// + GETSTATIC Test$.inEC : Ljava/lang/String;
//
// And the GETSTATIC blocks.
println("got it")
}
}
t.start()
t.join()
println("done")
}
testOnSuccessCustomEC()
}
object Test extends CustomExecutionContext {
def main(args: Array[String]): Unit = {
}
}
This is reminiscent of the deadlocks that became possible with the 2.12 lambda encoding. Prior to that, lambdas were inner classes, which had access to the module instance field via the outer pointer. Now, inner classes, even though they have the module instance, are exposed to the static init lock when the call a field accessor method. |
|
Another thing that's changed in the serialized form of top level modules: This confuses me. The following details of Aren't these enough to keep serialization stable? |
|
Ah, looking at the implementation of serialization, it records the descriptors of instance fields, even when So functionally, we have forwards and backwards serialization compatibility, but the irrelevent deatils in the serialization format make it hard for our tests to show that. An alternative would be to serialize all top level modules package scala.runtime;
public final class SerializedObject {
private final Class<?> moduleClass;
private static final ClassValue<Object> instances = new ClassValue<Object>() {
@Override
protected Object computeValue(Class<?> type) {
try {
return type.getField("MODULE$").get(null);
} catch (IllegalAccessException | NoSuchFieldException e) {
throw new RuntimeException(e);
}
}
};
public SerializedObject(Class<?> moduleClass) {
this.moduleClass = moduleClass;
}
private Object readResolve() {
return instances.get(moduleClass);
}
} |
retronym
force-pushed
the
topic/static-object-fields
branch
2 times, most recently
from
f97e9b2 to
e9fd345
Compare
|
Here's a demo that shows that object val members can now be JIT inlined: import java.lang.invoke.{MethodHandle, MethodHandles, MethodType}
class C
object Test {
val handle = MethodHandles.lookup().findVirtual(Test.getClass, "callMe", MethodType.methodType(classOf[Object]))
def newObject2 = handle.invoke(this)
@volatile var z = 0
def main(args: Array[String]): Unit = {
while(true) {
z += newObject2.hashCode()
}
}
def callMe = new Object
}Before: |
|
Benchmarks points to appear here. The first will show the improvement from using this PR as STARR, rather than the preceding commit. The second point will show the subsequent change from using this PR as STARR and this PR as the compiler-under-performance test, to see if this extra work in the backend makes a difference. |
| @@ -37,7 +37,7 @@ trait TestBase { | |||
| } | |||
|
|
|||
|
|
|||
| trait FutureCallbacks extends TestBase { | |||
| class FutureCallbacks extends TestBase { | |||
There was a problem hiding this comment.
@retronym Out of curiosity, why did these need to change?
There was a problem hiding this comment.
As written, the tests ran during the Test$.<clinit>, during which time they started threads that called back into accessors of mixed-in fields (like inEc) in Test$. That didn't deadlock because those fields were non-static and the reference wasn't made via the static Test$.MODULE$, but rather through the outer$ reference of an inner class in the mixed-in trait.
After this PR, pattern leads to a deadlock: the <clinit> thread is waiting for the other thread to stop, and the other thread is blocked on the GETSTATIC instruction.
There was a problem hiding this comment.
@retronym That makes me feel skeptical about this encoding. It would be interesting to see the community build running on this PR.
There was a problem hiding this comment.
Yeah, it's unfortunate. But code that publishes this for use on other threads before initialization is complete is asking for trouble, and tends to run afoul of subtle differences in the way things are compiled. We had the same problem with the lambda encoding on 2.12.
There was a problem hiding this comment.
@retronym Let's see what community-build says :)
| } | ||
| val (pre, rest0) = stats span (!isConstr(_)) | ||
| val (supercalls, rest) = rest0 span (isConstr(_)) | ||
| (pre ::: supercalls, rest) |
There was a problem hiding this comment.
This seems like a lot of iteration and a lot of allocation. Would this not be possible to solve with a custom Ordering + a sort?
There was a problem hiding this comment.
Yes it could be done with a bit less garbage, but this isn't a hotspot based on profiles I've collected.
There was a problem hiding this comment.
It's your call :) I always err on the side of efficiency :)
| val constructorDefDef = treeInfo.firstConstructor(cd0.impl.body).asInstanceOf[DefDef] | ||
| val (uptoSuperStats, remainingConstrStats) = treeInfo.splitAtSuper(constructorDefDef.rhs.asInstanceOf[Block].stats, classOnly = true) | ||
| val clInitSymbol = claszSymbol.newMethod(TermName("<clinit>"), claszSymbol.pos, Flags.STATIC).setInfo(NullaryMethodType(definitions.UnitTpe)) | ||
| val moduleField = claszSymbol.newValue(nme.MODULE_INSTANCE_FIELD, claszSymbol.pos, Flags.STATIC | Flags.PRIVATE).setInfo(claszSymbol.tpeHK) |
There was a problem hiding this comment.
It's maybe a little confusing to create the field symbol but not enter it in the decls and rely on addModuleInstanceField. A comment would be helpful.
(Class fields are generated based on BCodeHelpers.fieldSymbols, so on the decls. Methods on the other hand are generated based on DefDefs.)
src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala
Outdated
Show resolved
Hide resolved
| // pain for scala-{js, native}? | ||
|
|
||
| for (f <- fieldSymbols(claszSymbol)) { | ||
| f.setFlag(Flags.STATIC) |
There was a problem hiding this comment.
It's a little bit funny that this is enough to make code gen emit getstatic/putstatic and avoid loading the module instance. The trees accessing the fields still have the shape Select(This("T"), "y"). It's fine with me; we don't really have a "correct" way to represent a static selection in trees, I think we discussed this before.
|
I asked Dmitry about this encoding since he spent some time on this problem a while ago, he said:
|
retronym
force-pushed
the
topic/static-object-fields
branch
from
e9fd345 to
f7cadba
Compare
|
Rebased on #7297, which locks down the serialized form of top level modules, making it insensitive to the changes to field modifiers. |
retronym
force-pushed
the
topic/static-object-fields
branch
from
f7cadba to
c14e477
Compare
Bzzzt, wrong again, me. The java serialiazation for ;tl;dr Going forwards, we will no longer incur checkfile updates to the serialization stability test when we add or remove fields. But we need one last update now! |
retronym
force-pushed
the
topic/static-object-fields
branch
from
c14e477 to
5ce1767
Compare
retronym
force-pushed
the
topic/static-object-fields
branch
from
5ce1767 to
423816d
Compare
|
Rebased. |
|
There appear to be unrelated failures in the community build run above related to collections changes and scala-java8-compat. I've re-run as https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/1522/ after rebasing this PR. I've also prepared a backport to 2.12.x to facilitate testing with a cleaner, and more comphrehensive (?) community build. |
confirm, unrelated |
|
is there a performance run with this PR? |
1 similar comment
|
is there a performance run with this PR? |
This lets us make the MODULE$ field unconditionally final,
which improves runtime performance marginally by letting
JIT elide null checks.
Fields of objects themselves and now also be JIT-inlinable
constants.
=====================================================================
Adapts scala-concurrent-tck.scala to avoid a new deadlock. This
pattern of code will deadlock if module fields are static.
```
trait CustomExecutionContext {
val inEC = ""
def testOnSuccessCustomEC(): Unit = {
val t = new Thread() {
override def run(): Unit = {
println("getting it")
inEC.length
// this.outer$.inEC() now blocks until the `Test$.<clinit>` has concluded, because `Test$.inEC()`
// now differs:
// public inEC()Ljava/lang/String;
// - ALOAD 0
// - GETFIELD Test$.inEC : Ljava/lang/String;
// + GETSTATIC Test$.inEC : Ljava/lang/String;
//
// And the GETSTATIC blocks.
println("got it")
}
}
t.start()
t.join()
println("done")
}
testOnSuccessCustomEC()
}
object Test extends CustomExecutionContext {
def main(args: Array[String]): Unit = {
}
}
```
We accepted a similar change as an acceptable consequence of the new
lambda encoding.
retronym
force-pushed
the
topic/static-object-fields
branch
from
5052d10 to
5e109ff
Compare
|
@lrytz I've pushed a change that only allows |
SethTisue
added
performance
maybe scala/scala#7270 ? ``` package example import scala.reflect.NameTransformer object A{ Main.a = 1 } object B{ Main.b = 1 } object Main { var a: Int = 0 var b: Int = 0 def initializeByConstructor(): Int = { this.a = 0 val List(constructor) = Class.forName("example.A$").getDeclaredConstructors.toList constructor.setAccessible(true) constructor.newInstance() val result = Main.a this.a = 0 result } def initializeByMODULE_field(): Int = { this.b = 0 Class.forName("example.B$").getDeclaredField(NameTransformer.MODULE_INSTANCE_NAME).get(null) val result = Main.b this.b = 0 result } def main(args: Array[String]): Unit = { println((A, B)) println(scala.util.Properties.versionString) println(initializeByConstructor()) println(initializeByMODULE_field()) } } ``` ``` [info] version 2.12.11 [info] 1 [info] 0 ``` ``` [info] version 2.13.2 [info] 0 [info] 0 ```
maybe scala/scala#7270 ? ``` package example import scala.reflect.NameTransformer object A{ Main.a = 1 } object B{ Main.b = 1 } object Main { var a: Int = 0 var b: Int = 0 def initializeByConstructor(): Int = { this.a = 0 val List(constructor) = Class.forName("example.A$").getDeclaredConstructors.toList constructor.setAccessible(true) constructor.newInstance() val result = Main.a this.a = 0 result } def initializeByMODULE_field(): Int = { this.b = 0 Class.forName("example.B$").getDeclaredField(NameTransformer.MODULE_INSTANCE_NAME).get(null) val result = Main.b this.b = 0 result } def main(args: Array[String]): Unit = { println((A, B)) println(scala.util.Properties.versionString) println(initializeByConstructor()) println(initializeByMODULE_field()) } } ``` ``` [info] version 2.12.11 [info] 1 [info] 0 ``` ``` [info] version 2.13.2 [info] 0 [info] 0 ```
This is a port of the following PR from Scala 2: scala/scala#7270
This is a port of the following PR from Scala 2: scala/scala#7270
This is a port of the following PR from Scala 2: scala/scala#7270
The problem can be seen from the following example:
trait A { def foo() = ??? }
trait B { def foo() = ??? }
object C extends A with B {
super[A].foo()
super[B].foo()
}
In the code above, we cannot translate the following calls
from <init> to <clinit>:
super[A].foo()
super[B].foo()
super[A].$iinit$()
super[B].$init$()
More details can be found here: scala#5928
A principled way would be to generage super accessors as it
is done in posttyper.
However, the backend has a magic to support
prefix to super trees in [1], which is exploited
in the Scala 2 fix [2].
[1] scala/scala#5944
[2] scala/scala#7270
This is a port of the following PR from Scala 2: scala/scala#7270
The problem can be seen from the following example:
trait A { def foo() = ??? }
trait B { def foo() = ??? }
object C extends A with B {
super[A].foo()
super[B].foo()
}
In the code above, we cannot translate the following calls
from <init> to <clinit>:
super[A].foo()
super[B].foo()
super[A].$iinit$()
super[B].$init$()
More details can be found here: scala#5928
A principled way would be to generage super accessors as it
is done in posttyper.
However, the backend has a magic to support
prefix to super trees in [1], which is exploited
in the Scala 2 fix [2].
[1] scala/scala#5944
[2] scala/scala#7270
This is a port of the following PR from Scala 2: scala/scala#7270
The problem can be seen from the following example:
trait A { def foo() = ??? }
trait B { def foo() = ??? }
object C extends A with B {
super[A].foo()
super[B].foo()
}
In the code above, we cannot translate the following calls
from <init> to <clinit>:
super[A].foo()
super[B].foo()
super[A].$iinit$()
super[B].$init$()
More details can be found here: scala#5928
A principled way would be to generage super accessors as it
is done in posttyper.
However, the backend has a magic to support
prefix to super trees in [1], which is exploited
in the Scala 2 fix [2].
[1] scala/scala#5944
[2] scala/scala#7270
This is a port of the following PR from Scala 2: scala/scala#7270
The problem can be seen from the following example:
trait A { def foo() = ??? }
trait B { def foo() = ??? }
object C extends A with B {
super[A].foo()
super[B].foo()
}
In the code above, we cannot translate the following calls
from <init> to <clinit>:
super[A].foo()
super[B].foo()
super[A].$iinit$()
super[B].$init$()
More details can be found here: scala#5928
A principled way would be to generage super accessors as it
is done in posttyper.
However, the backend has a magic to support
prefix to super trees in [1], which is exploited
in the Scala 2 fix [2].
[1] scala/scala#5944
[2] scala/scala#7270
This is a port of the following PR from Scala 2: scala/scala#7270
The problem can be seen from the following example:
trait A { def foo() = ??? }
trait B { def foo() = ??? }
object C extends A with B {
super[A].foo()
super[B].foo()
}
In the code above, we cannot translate the following calls
from <init> to <clinit>:
super[A].foo()
super[B].foo()
super[A].$iinit$()
super[B].$init$()
More details can be found here: scala#5928
A principled way would be to generage super accessors as it
is done in posttyper.
However, the backend has a magic to support
prefix to super trees in [1], which is exploited
in the Scala 2 fix [2].
[1] scala/scala#5944
[2] scala/scala#7270
This is a port of the following PR from Scala 2: scala/scala#7270
The problem can be seen from the following example:
trait A { def foo() = ??? }
trait B { def foo() = ??? }
object C extends A with B {
super[A].foo()
super[B].foo()
}
In the code above, we cannot translate the following calls
from <init> to <clinit>:
super[A].foo()
super[B].foo()
super[A].$iinit$()
super[B].$init$()
More details can be found here: scala#5928
A principled way would be to generage super accessors as it
is done in posttyper.
However, the backend has a magic to support
prefix to super trees in [1], which is exploited
in the Scala 2 fix [2].
[1] scala/scala#5944
[2] scala/scala#7270
This lets us make the MODULE$ field unconditionally final,
which improves runtime performance marginally by letting
JIT elide null checks.
Fields of objects themselves and now also be JIT-inlinable
constants.
Fixes scala/scala-dev#537