Skip to content

Fix and extend Array.apply varargs optimization #9356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

retronym
Copy link
Member

@retronym retronym commented Dec 2, 2020

The existing optimization for Array.apply[T] overreaches by assuming that the ArrayValue carrying the varargs will have the same element type as the implicit class tag. However, this is not the case when the type argument is a generic type:

scala> def foo[T <: AnyRef : reflect.ClassTag](t: T) = Array.apply[T](t); foo[String]("")
java.lang.ClassCastException: [Ljava.lang.Object; cannot be cast to [Ljava.lang.String;
  ... 32 elided

This PR limits the existing optimization to cases where:

  • The class tag is an implicit arg
  • The class tag is materialized (not found via normal implicit search)

The second criterion excludes the call above.

A new, fallback optimization is provided that avoids a temporary
array and collection wrapper by emitting:

val arr = classTag.newArray(N)
ScalaRunTime.arrayUpdate(arr, N, elemN)
...
arr

Fixes scala/bug#11882

@scala-jenkins scala-jenkins added this to the 2.12.14 milestone Dec 2, 2020
@retronym
Copy link
Member Author

retronym commented Dec 2, 2020

Found a long standing bug in the existing optimization in the process!

@lrytz
Copy link
Member

lrytz commented Dec 3, 2020

% scalac t4216.scala
% <in process execution of run/t4216.scala> > t4216-run.log
% diff /home/jenkins/workspace/scala-2.12.x-validate-main@3/test/files/run/t4216.check /home/jenkins/workspace/scala-2.12.x-validate-main@3/test/files/run/t4216-run.log
@@ -9,7 +9,9 @@ scala> f(".")
 res0: java.util.List[String] = [.]
 
 scala> f(0)
-res1: java.util.List[Int] = [0]
+java.lang.ClassCastException: [I cannot be cast to [Ljava.lang.Object;
+  at .f(<console>:12)
+  ... 63 elided

@retronym retronym force-pushed the ticket/11882 branch 3 times, most recently from 969d2f1 to 971374c Compare December 4, 2020 06:08
The existing optimization for `Array.apply[T]` overreaches by
assuming that the `ArrayValue` carrying the varargs will have the
same element type as the implicit class tag. However, this is
not the case when the type argument is a generic type:

```
scala> def foo[T <: AnyRef : reflect.ClassTag](t: T) = Array.apply[T](t); foo[String]("")
java.lang.ClassCastException: [Ljava.lang.Object; cannot be cast to [Ljava.lang.String;
  ... 32 elided
```

This PR limits the existing optimization to cases where:

  - The class tag is an implicit arg
  - The class tag is materialized (not found via normal implicit search)

The second criterion excludes the call above.

A new, fallback optimization is provided that avoids a temporary
array and collection wrapper by emitting:

```
val arr = classTag.newArray(N)
ScalaRunTime.arrayUpdate(arr, N, elemN)
...
arr
}
```
@retronym retronym changed the title Extend Array.apply optimization to generic arrays Fix and extend Array.apply varargs optimization Dec 4, 2020
@retronym
Copy link
Member Author

retronym commented Dec 4, 2020

scala> def concrete = Array.apply[String]("")
concrete: Array[String]

scala> :javap concrete#concrete
  public java.lang.String[] concrete();
    descriptor: ()[Ljava/lang/String;
    flags: ACC_PUBLIC
    Code:
      stack=4, locals=1, args_size=1
         0: iconst_1
         1: anewarray     #21                 // class java/lang/String
         4: dup
         5: iconst_0
         6: ldc           #23                 // String
         8: aastore
         9: checkcast     #25                 // class "[Ljava/lang/Object;"
        12: checkcast     #27                 // class "[Ljava/lang/String;"
        15: areturn
      LineNumberTable:
        line 11: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0      16     0  this   L$line3/$read$$iw$$iw$;
scala> def generic[T <: AnyRef: reflect.ClassTag](t: T) = Array.apply[T](t); generic[String]("")
generic: [T <: AnyRef](t: T)(implicit evidence$1: scala.reflect.ClassTag[T])Array[T]
res3: Array[String] = Array("")

scala> :javap generic#generic
  public <T extends java.lang.Object> T[] generic(T, scala.reflect.ClassTag<T>);
    descriptor: (Ljava/lang/Object;Lscala/reflect/ClassTag;)[Ljava/lang/Object;
    flags: ACC_PUBLIC
    Code:
      stack=4, locals=4, args_size=3
         0: aload_2
         1: iconst_1
         2: invokeinterface #30,  2           // InterfaceMethod scala/reflect/ClassTag.newArray:(I)Ljava/lang/Object;
         7: astore_3
         8: getstatic     #35                 // Field scala/runtime/ScalaRunTime$.MODULE$:Lscala/runtime/ScalaRunTime$;
        11: aload_3
        12: iconst_0
        13: aload_1
        14: invokevirtual #39                 // Method scala/runtime/ScalaRunTime$.array_update:(Ljava/lang/Object;ILjava/lang/Object;)V
        17: aload_3
        18: checkcast     #41                 // class "[Ljava/lang/Object;"
        21: areturn
      LineNumberTable:
        line 11: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0      22     0  this   L$line7/$read$$iw$$iw$;
            0      22     1     t   Ljava/lang/Object;
            0      22     2 evidence$1   Lscala/reflect/ClassTag;
    Signature: #22                          // <T:Ljava/lang/Object;>(TT;Lscala/reflect/ClassTag<TT;>;)[TT;
    MethodParameters:
      Name                           Flags
      t                              final
      evidence$1                     final

@retronym retronym requested a review from sjrd December 4, 2020 07:30
@retronym
Copy link
Member Author

retronym commented Dec 4, 2020

@lrytz I've reworked this a little.

@retronym

This comment has been minimized.

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume I was tagged as reviewer for the interaction with Scala.js. Don't worry: Scala.js runs before cleanup, so this PR won't affect Scala.js.

Incidentally, I would like it if this particular optimization for Array.apply would happen earlier in the pipeline, so that we could benefit from it for free. Currently we replicate it in our back-end. ;)

@retronym
Copy link
Member Author

retronym commented Dec 6, 2020 via email

@retronym
Copy link
Member Author

retronym commented Dec 6, 2020 via email

@SethTisue
Copy link
Member

@retronym is this ready for merge, or do you want to explore the possibility of moving this to earlier in the pipeline here in Scala 2?

@retronym retronym marked this pull request as ready for review January 25, 2021 05:11
@retronym retronym merged commit f854b0f into scala:2.12.x Jan 25, 2021
@retronym
Copy link
Member Author

I'm happy enough with the solution here, so booking the progress!

@SethTisue
Copy link
Member

regression? see scala/scala-dev#760

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants