Fix #1167: Reduce the magic in Arrays.newRefArray. Implement multidimensional arrays #1188

sjrd · 2016-03-23T11:24:30Z

Previously, the method Arrays.newRefArray was one of the only 3
methods that are kept generic after erasure. This commit removes
this magic, by making it take an actual j.l.Class[T] as
parameter.

Moreover, the methods newXArray all receive an actual body,
implemented on top of Java reflection, which means that a back-end
does not have to special-case those methods for correctness.
It might still be required for performance, though, depending on
the back-end.

The JVM back-end is made non-optimal in this commit, precisely
because it does not specialize that method anymore. Doing so
requires modifying the fork of scalac that we use, which should
be done separately.

The JS back-end is adapted simply by doing nothing at all on any
of the newXArray methods. It will normally call the user-space
implementations which use reflection. The Scala.js optimizer will
inline and intrinsify the reflective calls, producing optimal
code, at the end of the day.

retronym · 2016-03-23T12:07:29Z

While you are in this area, it might be good to consider whether and how to extend array support to use multinewarray for Array.ofDim.

public class Test {
    public static void main(String... args) {
        int[][] multi = new int[5][10];
    }
}

  public static void main(java.lang.String...);
    Code:
       0: iconst_5
       1: bipush        10
       3: multianewarray #2,  2             // class "[[I"
       7: astore_1
       8: return

vs

  public <T> java.lang.Object[] ofDim(int, int, scala.reflect.ClassTag<T>);
    Code:
       0: getstatic     #319                // Field scala/reflect/ClassTag$.MODULE$:Lscala/reflect/ClassTag$;
       3: getstatic     #101                // Field scala/runtime/ScalaRunTime$.MODULE$:Lscala/runtime/ScalaRunTime$;
       6: aload_3
       7: invokeinterface #322,  1          // InterfaceMethod scala/reflect/ClassTag.runtimeClass:()Ljava/lang/Class;
      12: invokevirtual #326                // Method scala/runtime/ScalaRunTime$.arrayClass:(Ljava/lang/Class;)Ljava/lang/Class;
      15: invokevirtual #329                // Method scala/reflect/ClassTag$.apply:(Ljava/lang/Class;)Lscala/reflect/ClassTag;
      18: iload_1
      19: invokeinterface #152,  2          // InterfaceMethod scala/reflect/ClassTag.newArray:(I)Ljava/lang/Object;
      24: checkcast     #330                // class "[Ljava/lang/Object;"
      27: astore        4
      29: getstatic     #335                // Field scala/runtime/RichInt$.MODULE$:Lscala/runtime/RichInt$;
      32: getstatic     #340                // Field scala/Predef$.MODULE$:Lscala/Predef$;
      35: iconst_0
      36: invokevirtual #344                // Method scala/Predef$.intWrapper:(I)I
      39: iload_1
      40: invokevirtual #348                // Method scala/runtime/RichInt$.until$extension0:(II)Lscala/collection/immutable/Range;
      43: iload_2
      44: aload_3
      45: aload         4
      47: invokedynamic #356,  0            // InvokeDynamic #10:apply$mcVI$sp:(ILscala/reflect/ClassTag;[Ljava/lang/Object;)Lscala/runtime/java8/JFunction1$mcVI$sp;
      52: invokevirtual #361                // Method scala/collection/immutable/Range.foreach$mVc$sp:(Lscala/Function1;)V
      55: aload         4
      57: areturn

sjrd · 2016-03-23T12:25:55Z

@retronym IIUC, this requires non-trivial changes in the back-end. IIRC, @DarkDimius wants to keep the back-end as close as possible to the scalac one.

sjrd · 2016-03-23T12:27:28Z

Review by @DarkDimius ?
Note: since I have no control over the scalac fork with the shared back-end, I cannot implement the last bit to recover efficiency, which is to specialize newRefArray(classOf[T], len) in the back-end.

DarkDimius · 2016-03-23T12:30:41Z

I'm very hesitant to accept the original change at the moment.
It would require either very ugly hacks in DottyBackendInterace(creating ill-typed trees on the fly) or modifying shared backed for more dotty specific stuff.

I have a feeling that we're getting a very small win: there are still generic methods after erasure. It does not give any win for any phases after erasure, except JS backend. But instead it would complicate shared backed a lot.

@retronym @sjrd GenBCode does support emitting multianewarray in both scalac and Dotty: https://github.com/DarkDimius/scala/blob/sharing-backend/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala#L633 . Though AFAIK this is dead code simply because neither dotty nor scalac has a tree that would reach this if (it expects a new*Array with more that 1 argument).

DarkDimius · 2016-03-23T12:33:02Z

More precisely, it allows to remove 15 quite simple lines lines from JS backend while adding 70+ lines of hard-to-maintain-between-frontends code to JVM shared backend.

DarkDimius · 2016-03-23T12:35:39Z

src/dotty/tools/dotc/ast/tpd.scala

+    else {
+      val typeApplied = newArr("Ref").appliedToTypeTrees(typeArg :: Nil)
+      val classOfArg =
+        ref(defn.Predef_classOf).appliedToTypeTrees(typeArg :: Nil)


This code would only work before classOf that is very early in Dotty(before 'patmat').

It should instead use tpd.clsOf(typeArg)

tpd.clsOf(typeArg) does not work. I tried that first. It choked in pickling because the pickler cannot pickle a JavaArrayType.

well this code also does not work for 80% of pipeline(it broke my optimization phases)
This code would not pass Ycheck after erasure either.

So ... tpd.clsOf(typeArg) is unreliable before pickling, and classOf[typeArg] is unreliable after ClassOf. Is there anything that works all the time to generate a literal classOf[T]? Should I fix clsOf so that it is reliable along the entire pipeline?

Should I fix clsOf so that it is reliable along the entire pipeline?

I guess it would be the way to go. It should check if the type it is given derives from scala.Array, if it does, only don't yet erase its type before erasure.

DarkDimius · 2016-03-23T12:42:09Z

On the other side: this PR makes
newRefArray very special amoung newXXXArray methods: it has different signature and needs to be handled specially.
I we decide to get this in, I would argue that we shouldn't have 2 kinds of array-construction methods.
We should only keep newRefArray as now it can create primitive arrays as well as ref-arrays.

sjrd · 2016-03-23T12:42:44Z

70+ lines of hard-to-maintain-between-frontends code to shared backend.

Hum ... I'm expecting this to be around 3 lines:

        case Apply(_, Constant(Literal(elemTpe: Type)) :: args) if isSyntheticArrayConstructor(app.symbol) =>
          generatedType = ArrayBType(toTypeKind(elemTpe))
          mkArrayConstructorCall(generatedType, app, args)

right before the line you were pointing at in your comment.

it allows to remove 15 quite simple lines lines from JS backend

I don't care about those. I care about https://github.com/lampepfl/dotty/pull/1188/files#diff-7d76351125e3a0376bea4b9db34a05ccL471

sjrd · 2016-03-23T12:44:22Z

We remove other, only keeping newRefArray as now it can create primitive arrays as well as ref-arrays.

Makes sense.

DarkDimius · 2016-03-23T12:49:59Z

Apply(_, Constant(Literal(elemTpe: Type)) :: args)

would not work without extending SharedBackendInterface.
Currently you cannot test if something is a Type. Type is just an abstract type alias.
elemTpe: Type would be erazed to elemTpe: Object and would always succeed.

sjrd · 2016-03-23T12:55:16Z

OK, then it can be done as in genConstant:

        case Apply(_, Constant(const) :: args)
            if isSyntheticArrayConstructor(app.symbol) && const.tag == ClazzTag =>
          generatedType = ArrayBType(toTypeKind(const.typeValue))
          mkArrayConstructorCall(generatedType, app, args)

DarkDimius · 2016-03-23T12:55:38Z

@sjrd, let me try to play with this branch before Dotty meeting and let's discuss there.
I would try keeping only 1 newArray method and seeing how would it be handled in backed between scalac and Dotty.

DarkDimius · 2016-03-23T17:30:07Z

@retronym @sjrd GenBCode does support emitting multianewarray in both scalac and Dotty

https://github.com/scala/scala/pull/5060/files indicates that though code existed, it was never executed or tested.

DarkDimius · 2016-03-23T20:12:18Z

@retronym thx for suggestion. Implemented: b2215ed

DarkDimius · 2016-03-23T20:18:42Z

Waiting for scala-compiler fork to reach maven central.
Here is the link for those who need to update version by hand in the IDE: https://oss.sonatype.org/content/repositories/releases/me/d-d/scala-compiler/2.11.5-20160322-171045-e19b30b3cd/

Backwards INcompatible with previous version.

DarkDimius · 2016-03-23T22:18:29Z

/rebuild

DarkDimius · 2016-03-23T22:34:59Z

sbt.ResolveException: unresolved dependency: org.scala-sbt#launcher-interface;1.0.0-M1: not found

/rebuild

sjrd · 2016-03-24T14:03:30Z

src/dotty/runtime/Arrays.scala

-  def newUnitArray(length: Int): Array[Unit] =
-    jlr.Array.newInstance(classOf[scala.runtime.BoxedUnit], length).asInstanceOf[Array[Unit]]
+  def newArray[Arr](componentType: Class[_], returnType: Class[Arr], dimensions: Array[Int]): Arr =
+    jlr.Array.newInstance(componentType, dimensions: _*).asInstanceOf[Arr]


Why do you need returnType, here?

Also, why no bound? Right now componentType is completely unchecked; it might not be in sync. What about the following signature?

def newArray[T](componentType: Class[T], returnType: Class[Arrary[T]], dimensions: Array[Int]): Array[T] = jlr.Array.newInstance(componentType, dimensions: _*).asInstanceOf[Arrary[T]]

Why do you need returnType, here?

I need to recreate the returned type at backend. And this is the easiest way I've found to do this.

Also, why no bound? Right now componentType is completely unchecked; it might not be in sync. What about the following signature?

It's NOT in sync before erasure. When you're creating a Array[Int], where Int is a primitive, you would like to pass https://docs.oracle.com/javase/7/docs/api/java/lang/Integer.html#TYPE, that is the Class for primitive Int, but is statically typed as Class[java.lang.Integer].

that is the Class for primitive Int, but is statically typed as Class[java.lang.Integer].

Ouch! Why do we need this? Why can't we keep a Literal(Constant(defn.IntType))? That's how scalac does it, and I find it much more consistent.

sjrd · 2016-03-24T14:14:05Z

That's all I have at this point.

DarkDimius · 2016-03-30T17:39:50Z

@odersky @sjrd tests passed. Ready for review.

sjrd · 2016-03-31T13:04:45Z

src/dotty/runtime/Arrays.scala

@@ -22,35 +24,8 @@ object Arrays {
    arr
  }

-  /** Create an array of type T. T must be of form Array[E], with
-   *  E being a reference type.
+  /** Create an array of a reference type T.


This comment is obsolete.

sjrd · 2016-03-31T13:12:09Z

That's all I have.

odersky · 2016-04-01T12:45:39Z

src/dotty/tools/dotc/typer/Applications.scala

-      tpd.cpy.Apply(tree)(newArray(targ, tree.pos), args)
+
+      def newGenericArrayCall =
+        ref(defn.DottyArraysModule).select(defn.newGenericArrayMethod).withPos(tree.pos).appliedToTypeTrees(targs).appliedToArgs(args)


This looks more readable on multiple lines.

odersky · 2016-04-17T10:46:04Z

What's the status of this PR?

DarkDimius · 2016-04-17T11:04:45Z

I assumed it was merged long time ago. Will rebase&merge tomorrow.

Previously, the method `Arrays.newRefArray` was one of the only 3 methods that are kept generic after erasure. This commit removes this magic, by making it take an actual `j.l.Class[T]` as parameter. Moreover, the methods `newXArray` all receive an actual body, implemented on top of Java reflection, which means that a back-end does not *have to* special-case those methods for correctness. It might still be required for performance, though, depending on the back-end. The JVM back-end is made non-optimal in this commit, precisely because it does not specialize that method anymore. Doing so requires modifying the fork of scalac that we use, which should be done separately. The JS back-end is adapted simply by doing nothing at all on any of the newXArray methods. It will normally call the user-space implementations which use reflection. The Scala.js optimizer will inline and intrinsify the reflective calls, producing optimal code, at the end of the day.

This one is able to encode creation of array of any type and any dimension. Note, it does not handle value classes.

It's done in a separate ArrayConstructors phase now.

That knows that there exists only single magical array method.

The problem comes from JavaArrayTypes. They are invalid before erasure, and cannot be pickled, while Array[T] is invalid after erasure and should be erased.

It's needed in order to create calls to newGenricArray as it needs to infer the ClassTag.

This allowed to simplify the code in both Applications and tpd.newArray. Now, only creation of generic arrays is handled by typer. All other arrays are handled in ArrayConstructors phase.

They need to be created through their class tag.

scala#1188

DarkDimius · 2016-04-18T13:01:40Z

Error wrapping InputStream in GZIPInputStream: java.io.EOFException

/rebuild

…ses (support for scala#1188)

DarkDimius reviewed Mar 23, 2016
View reviewed changes

DarkDimius force-pushed the remove-newarray-magic branch from d5d92e7 to 0a09332 Compare March 23, 2016 17:43

sjrd reviewed Mar 24, 2016
View reviewed changes

DarkDimius changed the title ~~Fix #1167: Remove the magic from Arrays.newRefArray.~~ Fix #1167: Reduce the magic in Arrays.newRefArray. Implement multidimensional arrays Mar 30, 2016

DarkDimius self-assigned this Mar 30, 2016

DarkDimius force-pushed the remove-newarray-magic branch from 0452155 to 55174aa Compare March 30, 2016 15:57

sjrd reviewed Mar 31, 2016
View reviewed changes

odersky reviewed Apr 1, 2016
View reviewed changes

sjrd and others added 12 commits April 18, 2016 14:44

Implement loading static fields in the Scala.js back-end.

88baa04

Get rid of all new***Array methods expect one.

0211e56

This one is able to encode creation of array of any type and any dimension. Note, it does not handle value classes.

Decouple handling array constructors from typer.

7e2352a

It's done in a separate ArrayConstructors phase now.

Update to new version of DottyBackendInterface.

3f41bd6

That knows that there exists only single magical array method.

Erasure: Erase constant literals.

57fcea6

The problem comes from JavaArrayTypes. They are invalid before erasure, and cannot be pickled, while Array[T] is invalid after erasure and should be erased.

Add a test to check multidimenstionall arrays.

3fc2b6e

make tpd.JavaSeqLiteral return JavaSeqLiteral

2c370bd

Bring back convertNewArray into typer.

9d7db0c

It's needed in order to create calls to newGenricArray as it needs to infer the ClassTag.

Separate handling of genericArray creation from normal ones.

c74bb42

This allowed to simplify the code in both Applications and tpd.newArray. Now, only creation of generic arrays is handled by typer. All other arrays are handled in ArrayConstructors phase.

ArrayConstructors: don't optimise creating of multi-dim generic arrays.

55e2ae6

They need to be created through their class tag.

Address review comments.

0b1ca2d

scala#1188

DarkDimius force-pushed the remove-newarray-magic branch from a531c3b to 397926b Compare April 18, 2016 12:50

DarkDimius added a commit to dotty-staging/dotty that referenced this pull request Apr 18, 2016

Address review comments of scala#1188

397926b

DarkDimius force-pushed the remove-newarray-magic branch from 397926b to ec00f1a Compare April 18, 2016 12:50

DarkDimius added a commit to dotty-staging/dotty that referenced this pull request Apr 18, 2016

Address review comments of scala#1188

ec00f1a

Address review comments of scala#1188

5399fbe

DarkDimius force-pushed the remove-newarray-magic branch from ec00f1a to 5399fbe Compare April 18, 2016 13:01

DarkDimius merged commit 247e913 into scala:master Apr 18, 2016

VladimirNik mentioned this pull request Apr 19, 2016

[work in progress] Arrays of value classes #1228

Closed

VladimirNik added a commit to dotty-staging/dotty that referenced this pull request Apr 25, 2016

vcArrays: use newArray method while transforming arrays of value clas…

aeb6323

…ses (support for scala#1188)

VladimirNik added a commit to dotty-staging/dotty that referenced this pull request Apr 26, 2016

vcArrays: use newArray method while transforming arrays of value clas…

ded6261

…ses (support for scala#1188)

VladimirNik added a commit to dotty-staging/dotty that referenced this pull request May 4, 2016

vcArrays: use newArray method while transforming arrays of value clas…

2b8419a

…ses (support for scala#1188)

felixmulder mentioned this pull request May 17, 2016

Array.ofDim(>1) does not produce multianewarray instructions #1259

Closed

VladimirNik added a commit to dotty-staging/dotty that referenced this pull request Jun 2, 2016

vcArrays: use newArray method while transforming arrays of value clas…

c8afac3

…ses (support for scala#1188)

allanrenucci deleted the remove-newarray-magic branch December 14, 2017 19:24

Fix #1167: Reduce the magic in Arrays.newRefArray. Implement multidimensional arrays #1188

Fix #1167: Reduce the magic in Arrays.newRefArray. Implement multidimensional arrays #1188

Uh oh!

Conversation

sjrd commented Mar 23, 2016

Uh oh!

retronym commented Mar 23, 2016

Uh oh!

sjrd commented Mar 23, 2016

Uh oh!

sjrd commented Mar 23, 2016

Uh oh!

DarkDimius commented Mar 23, 2016

Uh oh!

DarkDimius commented Mar 23, 2016

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!

DarkDimius commented Mar 23, 2016

Uh oh!

sjrd commented Mar 23, 2016

Uh oh!

sjrd commented Mar 23, 2016

Uh oh!

DarkDimius commented Mar 23, 2016

Uh oh!

sjrd commented Mar 23, 2016

Uh oh!

DarkDimius commented Mar 23, 2016

Uh oh!

DarkDimius commented Mar 23, 2016

Uh oh!

DarkDimius commented Mar 23, 2016

Uh oh!

DarkDimius commented Mar 23, 2016

Uh oh!

DarkDimius commented Mar 23, 2016

Uh oh!

DarkDimius commented Mar 23, 2016

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!

sjrd commented Mar 24, 2016

Uh oh!

DarkDimius commented Mar 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrd commented Mar 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky commented Apr 17, 2016

Uh oh!

DarkDimius commented Apr 17, 2016

Uh oh!

DarkDimius commented Apr 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DarkDimius commented Apr 18, 2016 •

edited

Loading