Skip to content

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

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.

Sign up for GitHub

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 5 commits into from
Jan 23, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
46a9783
Fix #3675: don't unnecessarily wrap arrays when eliminating java varargs
Jan 18, 2018
7e18110
Add unit test and address review comments
Jan 19, 2018
09dbfc2
Pre-calculate the set of wrapXArray methods in Definitions
Jan 19, 2018
9c2f9c2
Use PerRun when calculating method symbols
Jan 19, 2018
5482916
Minor review fixes
Jan 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Definitions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import scala.collection.{ mutable, immutable }
import PartialFunction._
import collection.mutable
import util.common.alwaysZero
import dotty.tools.dotc.transform.TreeGen

object Definitions {

Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

def Predef_wrapArray(implicit ctx: Context): Set[Symbol] = Predef_wrapArrayR.map(_.symbol)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


lazy val ScalaRuntimeModuleRef = ctx.requiredModuleRef("scala.runtime.ScalaRunTime")
def ScalaRuntimeModule(implicit ctx: Context) = ScalaRuntimeModuleRef.symbol
Expand Down
10 changes: 1 addition & 9 deletions compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ 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) => // rewrite a call to `wrapXArray(arr)` to `arr`
case app@Apply(fun, args) if defn.Predef_wrapArray.contains(fun.symbol) => // rewrite a call to `wrapXArray(arr)` to `arr`
args.head
case _ =>
val elemType = tree.tpe.elemType
Expand All @@ -108,14 +108,6 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
// Because of phantomclasses, the Java array's type might not conform to the return type
}

/** Determines whether `tree` is a method call to Predef.wrapXArray */
private def isWrappedArray(tree: Apply)(implicit ctx: Context): Boolean = {
val elemTpe = tree.tpe.elemType
val wrapMethodName = TreeGen.wrapArrayMethodName(elemTpe)
val wrapMethodSym = defn.ScalaPredefModuleRef.denot.requiredMethod(wrapMethodName)
tree.fun.symbol == wrapMethodSym
}

override def transformTypeApply(tree: TypeApply)(implicit ctx: Context): Tree =
transformTypeOfTree(tree)

Expand Down