Fix #3675: don't unnecessarily wrap arrays when eliminating java varargs #3869

abeln · 2018-01-18T19:09:18Z

When eliminating varargs in a java method call, we need to convert Seqs
into Arrays. However, if the object we're passing is already a wrapped
array, we just need to unwrap it.

Testing

Added a unit test that checks that

import java.nio.file._
class Test {
  def test(xs: Array[String]) = {
     val p4 = Paths.get("Hello", xs: _*)
  }
}

now gives

val p4: java.nio.file.Path = java.nio.file.Paths.get("Hello", xs) (i.e. there's no call to wrapXArray)

abeln · 2018-01-18T19:14:14Z

compiler/src/dotty/tools/dotc/transform/TreeGen.scala

+    tree match {
+      case Apply(fun, _) =>
+        val sname = fun.symbol.name
+        sname == nme.wrapRefArray || sname == nme.wrapXArray(elemTpe.classSymbol.name)


@allanrenucci can you suggest a more robust way to identify whether the node corresponds to a wrapped arrray?

You can try something like:

private def isWrapArrayMethodCall(tree: Apply)(implicit ctx: Context): Boolean = { val elemType = tree.tpe.elemType val wrapArrayMethodSym = defn.ScalaPredefModuleRef.requiredMethodRef(wrapArrayMethodName(elemTpe)) tree.fun.symbol == wrapArrayMethodSym }

Also I would move this method to ElimRepeated

I would store the result of requiredMethodRef in a val in https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/core/Definitions.scala (see existing examples like https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/core/Definitions.scala#L339-L340)

Though here it's more complicated since there's one method per primitive type + Ref. You could store all of them in a Set in Definitions and then just check if the current symbol is contained in the Set.

… varargs When eliminating varargs in a java method call, we need to convert Seqs into Arrays. However, if the object we're passing is already a wrapped array, we just need to unwrap it. Testing ------- Eliminating varargs in ``` import java.nio.file._ class Test { def test(xs: Array[String]) = { val p4 = Paths.get("Hello", xs: _*) } } ``` now gives `val p4: java.nio.file.Path = java.nio.file.Paths.get("Hello", xs)`

abeln · 2018-01-19T18:44:06Z

compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala

@@ -106,6 +108,14 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
          // Because of phantomclasses, the Java array's type might not conform to the return type
  }

+  /** Determines whether the given `tree` represents a wrapped array */
+  private def isWrappedArray(tree: Apply)(implicit ctx: Context): Boolean = {


@allanrenucci That's much nicer! Thanks! PTAL

I think the name of the method and doc are confusing. This tests that the tree is a method call to Predef.wrappedXArray, not that the tree represents a wrapped array. I would prefer:

/** Is the tree a method call to Predef.wrapXArray */ def isWrapArrayMethodCall

allanrenucci

Can you add a test to compiler/test/dotty/tools/backend/jvm/DottyByteCodeTests.scala that compiles:

import java.nio.file._
class Test {
  def test(xs: Array[String]) = {
     val p4 = Paths.get("Hello", xs: _*)
  }
}

and make sure there is no call to wrapRefArray in the bytecode.

allanrenucci · 2018-01-19T18:54:26Z

compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala

@@ -106,6 +108,14 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
          // Because of phantomclasses, the Java array's type might not conform to the return type
  }

+  /** Determines whether the given `tree` represents a wrapped array */
+  private def isWrappedArray(tree: Apply)(implicit ctx: Context): Boolean = {


I think the name of the method and doc are confusing. This tests that the tree is a method call to Predef.wrappedXArray, not that the tree represents a wrapped array. I would prefer:

/** Is the tree a method call to Predef.wrapXArray */ def isWrapArrayMethodCall

allanrenucci · 2018-01-19T18:56:46Z

compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala

@@ -94,6 +94,8 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
  private def seqToArray(tree: Tree, pt: Type)(implicit ctx: Context): Tree = tree match {
    case SeqLiteral(elems, elemtpt) =>
      JavaSeqLiteral(elems, elemtpt)
+    case app@Apply(fun, args) if isWrappedArray(app) =>


I would remove the comment below and add a comment here:

// rewrite a call to `wrapXArray(arr)` to `arr`

abeln · 2018-01-19T19:44:56Z

compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala

+        }
+      }
+
+      assert(!arrayWrapped, "Arrays should not be wrapped when passed to a Java varargs method\n")


I verified that this fails if I comment out the changes to ElimRepeated.

abeln

PTAL

abeln · 2018-01-19T19:45:53Z

Thanks for the pointer to the BC unit tests. Didn't know they existed.

abeln · 2018-01-19T21:46:45Z

compiler/src/dotty/tools/dotc/core/Definitions.scala

@@ -338,6 +339,12 @@ class Definitions {
    def Predef_classOf(implicit ctx: Context) = Predef_classOfR.symbol
    lazy val Predef_undefinedR = ScalaPredefModule.requiredMethodRef("???")
    def Predef_undefined(implicit ctx: Context) = Predef_undefinedR.symbol
+    // The set of all wrap{X, Ref}Array methods, where X is a value type


@smarter pre-calculated the set of methods here, as suggested
PTAL

smarter · 2018-01-19T21:52:14Z

compiler/src/dotty/tools/dotc/core/Definitions.scala

+      val methodNames = ScalaValueTypes.map(TreeGen.wrapArrayMethodName) + nme.wrapRefArray
+      methodNames.map(ScalaPredefModule.requiredMethodRef).toSet
+    }
+    def Predef_wrapArray(implicit ctx: Context): Set[Symbol] = Predef_wrapArrayR.map(_.symbol)


To avoid redoing the .map(_.symbol) every time this method is called it would make sense to cache its result. Symbols are valid at least for one run so you can use the PerRun class for this, grep for PerRun in this file for some examples.

abeln · 2018-01-19T22:30:04Z

PTAL

allanrenucci

Otherwise LGTM. Thanks @abeln!

allanrenucci · 2018-01-20T15:25:56Z

compiler/src/dotty/tools/dotc/core/Definitions.scala

+    lazy val Predef_wrapArrayR: collection.Set[TermRef] = {
+      val methodNames = ScalaValueTypes.map(TreeGen.wrapArrayMethodName) + nme.wrapRefArray
+      methodNames.map(ScalaPredefModule.requiredMethodRef)
+    }


I would inline this in Predef_wrapArray since it is only used there:

val Predef_wrapArray = new PerRun[collection.Set[Symbol]] { implicit ctx => val methodNames = ScalaValueTypes.map(TreeGen.wrapArrayMethodName) + nme.wrapRefArray methodNames.map(name => ScalaPredefModule.requiredMethodRef(name).toSymbol) }

allanrenucci · 2018-01-20T15:28:07Z

compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala

+      val moduleNode = loadClassNode(moduleIn.input)
+      val method = getMethod(moduleNode, "test")
+
+      val arrayWrapped = instructionsFromMethod(method).exists { instr: Instruction =>


You can do:

val arrayWrapped = instructionsFromMethod(method).exists { case inv: Invoke => inv.name.contains("wrapRefArray") case _ => false }

abeln · 2018-01-22T23:24:51Z

Made requested changes. @allanrenucci PTAL
Thanks for patiently shepherding this!

abeln force-pushed the wrapped-array branch from 1dfddef to 6cc12df Compare January 18, 2018 19:11

abeln commented Jan 18, 2018

View reviewed changes

allanrenucci self-assigned this Jan 18, 2018

abeln force-pushed the wrapped-array branch from 6cc12df to 46a9783 Compare January 19, 2018 18:42

abeln commented Jan 19, 2018

View reviewed changes

allanrenucci suggested changes Jan 19, 2018

View reviewed changes

Add unit test and address review comments

7e18110

abeln commented Jan 19, 2018

View reviewed changes

Pre-calculate the set of wrapXArray methods in Definitions

09dbfc2

abeln commented Jan 19, 2018

View reviewed changes

smarter reviewed Jan 19, 2018

View reviewed changes

Use PerRun when calculating method symbols

9c2f9c2

allanrenucci approved these changes Jan 20, 2018

View reviewed changes

Minor review fixes

5482916

allanrenucci merged commit d691163 into scala:master Jan 23, 2018

abeln deleted the wrapped-array branch January 23, 2018 12:11

abeln restored the wrapped-array branch January 23, 2018 12:11

abeln deleted the wrapped-array branch January 23, 2018 12:11

abeln restored the wrapped-array branch January 23, 2018 12:12

Fix #3675: don't unnecessarily wrap arrays when eliminating java varargs #3869

Fix #3675: don't unnecessarily wrap arrays when eliminating java varargs #3869

Uh oh!

Conversation

abeln commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

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!

allanrenucci left a comment

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!

abeln left a comment

Choose a reason for hiding this comment

Uh oh!

abeln commented Jan 19, 2018

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!

abeln commented Jan 19, 2018

Uh oh!

allanrenucci left a comment

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!

abeln commented Jan 22, 2018

Uh oh!

Uh oh!

abeln commented Jan 18, 2018 •

edited

Loading