Fix lambda deserialization in classes with 252+ lambdas by retronym · Pull Request #5857 · scala/scala

retronym · 2017-04-19T01:38:07Z

Create a lambda deserializer per group of target methods,
and call these sequentially trapping the particular pattern
of exception that is thrown when a target method is absent
from the map.

Fixes scala/bug#10232

  // access flags 0x100A
  private static synthetic $deserializeLambda$(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object;
    TRYCATCHBLOCK L0 L1 L1 java/lang/IllegalArgumentException
   L0
    ALOAD 0
    INVOKEDYNAMIC lambdaDeserialize(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object; [
      // handle kind 0x6 : INVOKESTATIC
      scala/runtime/LambdaDeserialize.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/invoke/CallSite;
      // arguments:
      // handle kind 0x6 : INVOKESTATIC
      Test$.$anonfun$main$1$adapted(Lscala/Function1;)Ljava/lang/Object;,
      // handle kind 0x6 : INVOKESTATIC
      Test$.$anonfun$lambdas$1(Ljava/lang/Object;)Ljava/lang/String;,

      ...

      Test$.$anonfun$lambdas$249(Ljava/lang/Object;)Ljava/lang/String;,
      // handle kind 0x6 : INVOKESTATIC
      Test$.$anonfun$lambdas$250(Ljava/lang/Object;)Ljava/lang/String;
    ]
    ARETURN
   L1
    ALOAD 0
    INVOKEDYNAMIC lambdaDeserialize(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object; [
      // handle kind 0x6 : INVOKESTATIC
      scala/runtime/LambdaDeserialize.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/invoke/CallSite;
      // arguments:
      // handle kind 0x6 : INVOKESTATIC
      Test$.$anonfun$lambdas$251(Ljava/lang/Object;)Ljava/lang/String;,
      // handle kind 0x6 : INVOKESTATIC
      Test$.$anonfun$lambdas$252(Ljava/lang/Object;)Ljava/lang/String;,

      ...

      // handle kind 0x6 : INVOKESTATIC
      Test$.$anonfun$lambdas$256(Ljava/lang/Object;)Ljava/lang/String;
    ]
    ARETURN
    MAXSTACK = 2
    MAXLOCALS = 1

retronym · 2017-04-19T01:45:51Z

Unfortunately, this change uses exceptions as control flow for lambda#252 and above, but that seems to be the best way around the compatibility constraint that the newly generated code for $lambdaDeserialize$ should work when run with older standard libraries.

I've prototyped a change to avoid the overhead of filling in the stack trace of the IllegalArgumentException used for control flow (2.12.x...retronym:ticket/10232-stackless), but I'm reluctant to merge that it also relies on hacks to avoid requiring API changes to the runtime support.

lrytz · 2017-04-19T13:35:14Z

src/compiler/scala/tools/nsc/backend/jvm/analysis/BackendUtils.scala

+      mv.visitVarInsn(ASTORE, 1)
+      mv.visitVarInsn(ALOAD, 1)
+      mv.visitMethodInsn(INVOKEVIRTUAL, "java/lang/IllegalArgumentException", "getCause", "()Ljava/lang/Throwable;", false)
+      mv.visitJumpInsn(IFNULL, nextLabel(i))


should there be an ALOAD 1; ATHROW here?

Actually, I've just noticed that the second throw IllegalArgumentException in LambdaDeserializer is detritus that should have been removed in 131402f.

I'll remove that throw and then simplify this PR (no need to check getCause == null here, just swallow any IAE unconditionally in all but the terminal indy call.

lrytz · 2017-04-19T13:49:35Z

Looks good otherwise. I guess for 2.13 we can have a solution that doesn't use exceptional control flow? You could mark the commit [nomerge].

I should have removed the try/catch when I removed the code path that could throw that exception in 131402f.

retronym · 2017-04-20T06:24:49Z

Pushed the discussed simplification.

I guess for 2.13 we can have a solution that doesn't use exceptional control flow? You could mark the commit [nomerge].

I'd rather merge this into 2.13.x and then adapt the result, I've opened a ticket to track that: https://github.com/scala/scala-dev/issues/373

lrytz · 2017-04-20T06:42:35Z

Sure, that's fine too.

lrytz · 2017-04-20T06:58:10Z

src/compiler/scala/tools/nsc/backend/jvm/analysis/BackendUtils.scala

+    def nextLabel(i: Int) = if (i == numGroups - 2) terminalLabel else initialLabels(i + 1)
+
+    for ((label, i) <- initialLabels.iterator.zipWithIndex) {
+      mv.visitTryCatchBlock(label, nextLabel(i), nextLabel(i), "java/lang/IllegalArgumentException")


Can you add jlIllegalArgumentExceptionRef to CoreBTypes, like jlClassCastExceptionRef, then use .internalName? This ensures that the ClassBType is part of the classBTypeFromInternalName map, which is excpected for computing stack map frames during class writing.

lrytz · 2017-04-20T07:07:49Z

src/compiler/scala/tools/nsc/backend/jvm/analysis/BackendUtils.scala

+   *     case i: IllegalArgumentException => // swallow
+   *   }
+   *   </FOR>
+   *   return indy[scala.runtime.LambdaDeserialize.bootstrap, targetMethodGroup${NUM_GROUPS-1}](l)


the generated structure is more like

try return indy[scala.runtime.LambdaDeserialize.bootstrap, targetMethodGroup$0](l) catch { case i: IllegalArgumentException => try return indy[scala.runtime.LambdaDeserialize.bootstrap, targetMethodGroup$1](l) catch { case i: IllegalArgumentException => ... return indy[scala.runtime.LambdaDeserialize.bootstrap, targetMethodGroup${NUM_GROUPS-1}](l) } }

Is there a difference? In any case, your version is clearer, so I'll use that.

Just a detail to make the comment align better with the implementation.

lrytz · 2017-04-20T07:28:27Z

src/compiler/scala/tools/nsc/backend/jvm/CoreBTypes.scala

  lazy val jlCloneableRef            : ClassBType = classBTypeFromSymbol(JavaCloneableClass)        // java/lang/Cloneable
  lazy val jiSerializableRef         : ClassBType = classBTypeFromSymbol(JavaSerializableClass)     // java/io/Serializable
  lazy val jlClassCastExceptionRef   : ClassBType = classBTypeFromSymbol(ClassCastExceptionClass)   // java/lang/ClassCastException
+  lazy val jlIllegalArgExceptionRef  : ClassBType = classBTypeFromSymbol(IllegalArgExceptionClass)  // java/lang/ClassCastException


Comment is out of synch..

lrytz · 2017-04-20T07:28:30Z

src/compiler/scala/tools/nsc/backend/jvm/analysis/BackendUtils.scala

+    def nextLabel(i: Int) = if (i == numGroups - 2) terminalLabel else initialLabels(i + 1)
+
+    for ((label, i) <- initialLabels.iterator.zipWithIndex) {
+      mv.visitTryCatchBlock(label, nextLabel(i), nextLabel(i), coreBTypes.jlIllegalArgExceptionRef.internalName)


coreBTypes._ is already imported

Create a lambda deserializer per group of target methods, and call these sequentially trapping the particular pattern of exception that is thrown when a target method is absent from the map. Fixes scala/bug#10232 ``` // access flags 0x100A private static synthetic $deserializeLambda$(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object; TRYCATCHBLOCK L0 L1 L1 java/lang/IllegalArgumentException L0 ALOAD 0 INVOKEDYNAMIC lambdaDeserialize(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object; [ // handle kind 0x6 : INVOKESTATIC scala/runtime/LambdaDeserialize.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/invoke/CallSite; // arguments: // handle kind 0x6 : INVOKESTATIC Test$.$anonfun$main$1$adapted(Lscala/Function1;)Ljava/lang/Object;, // handle kind 0x6 : INVOKESTATIC Test$.$anonfun$lambdas$1(Ljava/lang/Object;)Ljava/lang/String;, ... Test$.$anonfun$lambdas$249(Ljava/lang/Object;)Ljava/lang/String;, // handle kind 0x6 : INVOKESTATIC Test$.$anonfun$lambdas$250(Ljava/lang/Object;)Ljava/lang/String; ] ARETURN L1 ALOAD 0 INVOKEDYNAMIC lambdaDeserialize(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object; [ // handle kind 0x6 : INVOKESTATIC scala/runtime/LambdaDeserialize.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/invoke/CallSite; // arguments: // handle kind 0x6 : INVOKESTATIC Test$.$anonfun$lambdas$251(Ljava/lang/Object;)Ljava/lang/String;, // handle kind 0x6 : INVOKESTATIC Test$.$anonfun$lambdas$252(Ljava/lang/Object;)Ljava/lang/String;, ... // handle kind 0x6 : INVOKESTATIC Test$.$anonfun$lambdas$256(Ljava/lang/Object;)Ljava/lang/String; ] ARETURN MAXSTACK = 2 MAXLOCALS = 1 ```

scala-jenkins added this to the 2.12.3 milestone Apr 19, 2017

retronym requested a review from lrytz April 19, 2017 01:45

retronym changed the title ~~Fix lambda serialization in classes with 252+ lambdas~~ Fix lambda deserialization in classes with 252+ lambdas Apr 19, 2017

retronym force-pushed the ticket/10232 branch from 4e09eb1 to be630ef Compare April 19, 2017 01:51

lrytz reviewed Apr 19, 2017

View reviewed changes

retronym force-pushed the ticket/10232 branch from be630ef to 7c5d1da Compare April 20, 2017 06:20

Remove dead code catch in LambdaDeserializer

d9343a7

I should have removed the try/catch when I removed the code path that could throw that exception in 131402f.

retronym force-pushed the ticket/10232 branch from 7c5d1da to 20587e9 Compare April 20, 2017 06:22

lrytz reviewed Apr 20, 2017

View reviewed changes

retronym force-pushed the ticket/10232 branch from 20587e9 to 043e31d Compare April 20, 2017 07:11

lrytz approved these changes Apr 20, 2017

View reviewed changes

retronym force-pushed the ticket/10232 branch from 043e31d to 9f522c6 Compare April 24, 2017 00:34

retronym merged commit 7cc1f86 into scala:2.12.x Apr 24, 2017

SethTisue mentioned this pull request Apr 24, 2017

Merge 2.12.x to 2.13.x [ci: last-only] #5862

Merged

retronym mentioned this pull request Jan 9, 2019

Adapt lambda deserialization fix in 2.13.x to avoid exceptions as control flow scala/bug#11345

Open

Conversation

retronym commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

retronym commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrytz commented Apr 19, 2017

Uh oh!

retronym commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrytz commented Apr 20, 2017

Uh oh!

lrytz Apr 20, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

retronym commented Apr 19, 2017 •

edited

Loading

retronym commented Apr 20, 2017 •

edited

Loading

lrytz Apr 20, 2017 •

edited

Loading