Use proxy-based serialization for static modules #6877

szeiger · 2018-07-02T15:25:46Z

This avoids serializing any state of serializable singletons which
would be discarded anyway after deserializing (SI-10412).

Fixes scala/bug#10412

This avoids serializing any state of serializable singletons which would be discarded anyway after deserializing (SI-10412).

lrytz

LGreatTM! Are there enough existing test to ensure deserialization gives the same instance?

mkeskells · 2018-07-02T21:19:12Z

src/compiler/scala/tools/nsc/typechecker/SyntheticMethods.scala

-     * readResolve.  Here it is implemented for all objects which have
-     * no implementation and which are marked serializable (which is true
-     * for all case objects.)
+     * readResolve.  Here we use a serialization proxy that prevents


I don't think the comment is quite right, how about

/* If you serialize a singleton you will get an additional * instance of the singleton, unless you implement * special serialization logic. Here we use a serialization proxy that prevents * serialization of state and will, on deserialization by replaced by the object * via use of readResolve. This is done for all top level objects which extend * `java.io.Serializable` (such as case objects) */

mkeskells · 2018-07-02T21:23:11Z

src/compiler/scala/tools/nsc/typechecker/SyntheticMethods.scala

      }
      def extras = {
-        if (needsReadResolve) {
+        if (needsModuleSerializationProxy) {


I do wonder if we should only generate this method if we are generating java class files. This is not required for js and native backends is it?

mkeskells · 2018-07-02T21:28:44Z

src/compiler/scala/tools/nsc/typechecker/SyntheticMethods.scala

         clazz.isModuleClass
      && clazz.isSerializable
-      && !hasConcreteImpl(nme.readResolve)
+      && !hasConcreteImpl(nme.writeReplace)


this is not the correct test.
We should be checking for a matching method, not a method with this name

It should not match def writeReplace(x:Int) = ???

We would need to rework significantly to avoid do a semantic check. We did manage to do that for Companion.apply recently, but that was far from easy. I'd suggest we take that up as a separate change.

mkeskells · 2018-07-02T21:32:58Z

src/compiler/scala/tools/nsc/typechecker/SyntheticMethods.scala

+        if (needsModuleSerializationProxy) {
          // Aha, I finally decoded the original comment.
          // This method should be generated as private, but apparently if it is, then
          // it is name mangled afterward.  (Wonder why that is.) So it's only protected.


This comment seems out of date - this method is (and was) private

is there some corresponding "don't mangle this name" setting somewhere

mkeskells · 2018-07-02T21:37:42Z

we also needs tests for the negative cases, where we don't generate the custom logic, e.g. if there is a custom writeReplace already, or this in not a top level object

retronym · 2018-07-02T22:56:26Z

Should we do this in two steps? Publish a new STARR, then make the changes to the library?

mkeskells · 2018-07-02T23:08:45Z

I think that there is a missing implementation for non top-level objects. They should be a singleton in their enclosing scope

In impemlementation - Shouldn't they defer to a field of an outer enclosing object?
To demo the problem in 2.12 you get this output

original inner Inner@71dac704 outer @123772c4 outer.Inner = Inner@71dac704 
after read     Inner@57829d67 outer @123772c4 outer.Inner = Inner@71dac704

from this test code

package x
import java.io._

object Test extends App{
  val baos = new ByteArrayOutputStream
  val oos = new ObjectOutputStream(baos)

  val inner = Cache.cached.Inner
  oos.writeObject(inner)

  val bytes = baos.toByteArray

  println(s"original inner $inner")

  val bais = new ByteArrayInputStream(bytes)
  val ois = new ObjectInputStream(bais)
  val x = ois.readObject

  println(s"after read     $x")
}

object Cache {
  val cached = Outer(3)
}
case class Outer(x: Int) extends Serializable {
  object Inner extends Serializable {
    override def toString = s"Inner@${System.identityHashCode(this).toHexString} outer @${System.identityHashCode(myOuter).toHexString} outer.Inner = Inner@${System.identityHashCode(outer.Inner).toHexString} "
    def myOuter = outer
  }
  def outer = this

  override def toString: String = super.toString

  def readResolve:AnyRef = {
    Cache.cached
  }
}

adriaanm · 2018-08-07T08:38:22Z

Can we use M5 as the new STARR? This sounds like something that should go in before the RC cycle, if we do it.

retronym · 2018-09-28T06:21:20Z

src/library/scala/runtime/ModuleSerializationProxy.scala

+@SerialVersionUID(1L)
+final class ModuleSerializationProxy(cls: Class[_]) extends Serializable {
+  private[this] def readResolve: AnyRef = cls.getField("MODULE$").get(null)
+}


We could extend this to cache the looked-up instances:

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); } }

The lookup in the ClassValue hashmap ought to be a fraction faster than the getField / get calls (although we'd need to benchmark to see if this matters.)

This would need to be classpath aware, or use WeakReference/WeakHashMap, otherwise it prevents class unloading

Indeed. I think ClassValue is weak in all the right places, but we should validate with a test case that dynamically loads an unloads classes.

Here's the performance difference:

[info] Benchmark Mode Cnt Score Error Units [info] ReflectBenchmark.getField avgt 20 3129.835 ± 54.981 ns/op [info] ReflectBenchmark.getFieldCached avgt 20 153.567 ± 13.023 ns/op

given:

package scala import java.util.concurrent.TimeUnit import org.openjdk.jmh.annotations._ import org.openjdk.jmh.infra._ object O1; object O2; object O3; object O4; object O5; object O6; object O7; object O8; object O9; object O10; object O11; object O12; object O13; object O14; object O15; object O16; object O17; object O18; object O19; object O20; object O21; object O22; object O23; object O24; object O25; object O26; object O27; object O28; object O29; object O30; object O31; object O32 @BenchmarkMode(Array(Mode.AverageTime)) @Fork(2) @Threads(1) @Warmup(iterations = 10) @Measurement(iterations = 10) @OutputTimeUnit(TimeUnit.NANOSECONDS) @State(Scope.Benchmark) class ReflectBenchmark { val classes = Array(O1.getClass, O2.getClass, O3.getClass, O4.getClass, O5.getClass, O6.getClass, O7.getClass, O8.getClass, O9.getClass, O10.getClass, O11.getClass, O12.getClass, O13.getClass, O14.getClass, O15.getClass, O16.getClass, O17.getClass, O18.getClass, O19.getClass, O20.getClass, O21.getClass, O22.getClass, O23.getClass, O24.getClass, O25.getClass, O26.getClass, O27.getClass, O28.getClass, O29.getClass, O30.getClass, O31.getClass, O32.getClass) val cache = new ClassValue[AnyRef] { override def computeValue(cls: Class[_]): AnyRef = { cls.getField("MODULE$").get(null) } } ////////////////////////////////////////////// @Benchmark def getField(bh: Blackhole): Unit = { val cls = classes var i = 0 while (i < cls.length) { bh.consume(cls(i).getField("MODULE$").get(null)) i += 1 } } @Benchmark def getFieldCached(bh: Blackhole): Unit = { val cls = classes var i = 0 while (i < cls.length) { bh.consume(cache.get(cls(i))) i += 1 } } }

Looks good on the weak value, front, too.

scala> def cl = new java.net.URLClassLoader(Array(new java.io.File("/tmp").toURI.toURL), getClass.getClassLoader) cl: java.net.URLClassLoader scala> val cache = new ClassValue[AnyRef] { def computeValue(cls: Class[_]) = cl.loadClass("O$").getField("MODULE$").get(null) } cache: ClassValue[AnyRef] = $anon$1@1de0641b scala> for (i <- (1 to 1000000)) {val cls = cl.loadClass("O$"); println(cache.get(cls)); System.gc(); } O$@714e861f O$@2fdf22f7 [Unloading class O$ 0x00000007c05b3428] [Unloading class O$ 0x00000007c05b2c28] O$@57e03347 [Unloading class O$ 0x00000007c05b4428] [Unloading class O$ 0x00000007c05b3c28] O$@b8a144e [Unloading class O$ 0x00000007c05b2c28] [Unloading class O$ 0x00000007c05b3428] O$@14b8a751 [Unloading class O$ 0x00000007c05b3c28] [Unloading class O$ 0x00000007c05b4428] O$@150d6eaf [Unloading class O$ 0x00000007c05b3428] [Unloading class O$ 0x00000007c05b2c28] O$@6615237

$ scala Welcome to Scala 2.12.4 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_172). Type in expressions for evaluation. Or try :help. scala> def cl = new java.net.URLClassLoader(Array(new java.io.File("/tmp").toURI.toURL), getClass.getClassLoader) cl: java.net.URLClassLoader scala> val cache = new ClassValue[AnyRef] { def computeValue(cls: Class[_]) = cl.loadClass("O$").getField("MODULE$").get(null) } cache: ClassValue[AnyRef] = $anon$1@3728a578 scala> while (true) {val cls = cl.loadClass("O$"); cache.get(cls); System.gc(); print(".") } ....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

retronym · 2018-10-01T23:20:08Z

@szeiger I've pushed my proposal to use ClassValue to #7297.

szeiger · 2018-10-10T15:15:39Z

Replaced by #7297

scala-jenkins added this to the 2.13.0-M5 milestone Jul 2, 2018

Use proxy-based serialization for modules

6f22b73

This avoids serializing any state of serializable singletons which would be discarded anyway after deserializing (SI-10412).

szeiger force-pushed the issue/10412 branch from 29f1a92 to 6f22b73 Compare July 2, 2018 16:00

lrytz reviewed Jul 2, 2018

View reviewed changes

mkeskells reviewed Jul 2, 2018

View reviewed changes

retronym changed the title ~~Use proxy-based serialization for modules~~ Use proxy-based serialization for static modules Jul 2, 2018

SethTisue modified the milestones: 2.13.0-M5, 2.13.0-RC1 Aug 7, 2018

retronym mentioned this pull request Sep 28, 2018

Make object fields static, and move post-super init to <clinit> #7270

Merged

retronym reviewed Sep 28, 2018

View reviewed changes

retronym mentioned this pull request Oct 1, 2018

Use proxy-based serialization for modules #7297

Merged

szeiger closed this Oct 10, 2018

SethTisue mentioned this pull request Oct 18, 2018

Serialization of static modules does not need to contain any data scala/bug#10412

Closed

SethTisue removed this from the 2.13.0-RC1 milestone Feb 22, 2019

Use proxy-based serialization for static modules #6877

Use proxy-based serialization for static modules #6877

Uh oh!

Conversation

szeiger commented Jul 2, 2018

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

mkeskells Jul 2, 2018 • edited by lrytz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkeskells commented Jul 2, 2018

Uh oh!

retronym commented Jul 2, 2018

Uh oh!

mkeskells commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriaanm commented Aug 7, 2018

Uh oh!

retronym Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

retronym commented Oct 1, 2018

Uh oh!

szeiger commented Oct 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mkeskells Jul 2, 2018 •

edited by lrytz

Loading

mkeskells commented Jul 2, 2018 •

edited

Loading

retronym Sep 28, 2018 •

edited

Loading