From dad886659faca4fba2d4937c9bc6780591b02c27 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Sat, 3 Nov 2012 13:34:20 +0100 Subject: SI-6611 Tighten up an unsafe array optimization The net was cast too wide and was unsafely optimizing away array copies. --- src/compiler/scala/tools/nsc/transform/CleanUp.scala | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'src/compiler/scala/tools/nsc/transform/CleanUp.scala') diff --git a/src/compiler/scala/tools/nsc/transform/CleanUp.scala b/src/compiler/scala/tools/nsc/transform/CleanUp.scala index 1f353bb31c..bdcebf47b8 100644 --- a/src/compiler/scala/tools/nsc/transform/CleanUp.scala +++ b/src/compiler/scala/tools/nsc/transform/CleanUp.scala @@ -15,6 +15,7 @@ abstract class CleanUp extends Transform with ast.TreeDSL { import global._ import definitions._ import CODE._ + import treeInfo.StripCast /** the following two members override abstract members in Transform */ val phaseName: String = "cleanup" @@ -618,14 +619,16 @@ abstract class CleanUp extends Transform with ast.TreeDSL { } transformApply - // This transform replaces Array(Predef.wrapArray(Array(...)), ) - // with just Array(...) - case Apply(appMeth, List(Apply(wrapRefArrayMeth, List(array)), _)) + // Replaces `Array(Predef.wrapArray(ArrayValue(...).$asInstanceOf[...]), )` + // with just `ArrayValue(...).$asInstanceOf[...]` + // + // See SI-6611; we must *only* do this for literal vararg arrays. + case Apply(appMeth, List(Apply(wrapRefArrayMeth, List(arg @ StripCast(ArrayValue(_, _)))), _)) if (wrapRefArrayMeth.symbol == Predef_wrapRefArray && appMeth.symbol == ArrayModule_overloadedApply.suchThat { - _.tpe.resultType.dealias.typeSymbol == ObjectClass + _.tpe.resultType.dealias.typeSymbol == ObjectClass // [T: ClassTag](xs: T*): Array[T] post erasure }) => - super.transform(array) + super.transform(arg) case _ => super.transform(tree) -- cgit v1.2.3 From 8265175ecc42293997d59049f430396c77a2b891 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Sun, 4 Nov 2012 14:17:25 +0100 Subject: Expand optimization of Array(e1, ..., en) to primitive arrays. --- .../scala/tools/nsc/transform/CleanUp.scala | 7 +++ src/library/scala/Array.scala | 10 ++++ .../scala/reflect/internal/Definitions.scala | 13 ++--- test/files/instrumented/t6611.scala | 24 ++++++++- test/files/run/t6611.scala | 63 ++++++++++++++++++++-- 5 files changed, 106 insertions(+), 11 deletions(-) (limited to 'src/compiler/scala/tools/nsc/transform/CleanUp.scala') diff --git a/src/compiler/scala/tools/nsc/transform/CleanUp.scala b/src/compiler/scala/tools/nsc/transform/CleanUp.scala index bdcebf47b8..4f145d3d7e 100644 --- a/src/compiler/scala/tools/nsc/transform/CleanUp.scala +++ b/src/compiler/scala/tools/nsc/transform/CleanUp.scala @@ -629,6 +629,13 @@ abstract class CleanUp extends Transform with ast.TreeDSL { _.tpe.resultType.dealias.typeSymbol == ObjectClass // [T: ClassTag](xs: T*): Array[T] post erasure }) => super.transform(arg) + case Apply(appMeth, List(elem0, Apply(wrapArrayMeth, List(rest @ ArrayValue(elemtpt, _))))) + if wrapArrayMeth.symbol == Predef_wrapArray(elemtpt.tpe) && + appMeth.symbol == ArrayModule_overloadedApply.suchThat { + tp => tp.tpe.paramss.flatten.lift.apply(1).exists(_.tpe.typeSymbol == SeqClass) && + tp.tpe.resultType =:= arrayType(elemtpt.tpe) // (p1: AnyVal1, ps: AnyVal1*): Array[AnyVal1] post erasure + } => + super.transform(rest.copy(elems = elem0 :: rest.elems)) case _ => super.transform(tree) diff --git a/src/library/scala/Array.scala b/src/library/scala/Array.scala index 0b8550be37..514844a5fa 100644 --- a/src/library/scala/Array.scala +++ b/src/library/scala/Array.scala @@ -115,6 +115,8 @@ object Array extends FallbackArrayBuilding { * @param xs the elements to put in the array * @return an array containing all elements from xs. */ + // Subject to a compiler optimization in Cleanup. + // Array(e0, ..., en) is translated to { val a = new Array(3); a(i) = ei; a } def apply[T: ClassTag](xs: T*): Array[T] = { val array = new Array[T](xs.length) var i = 0 @@ -123,6 +125,7 @@ object Array extends FallbackArrayBuilding { } /** Creates an array of `Boolean` objects */ + // Subject to a compiler optimization in Cleanup, see above. def apply(x: Boolean, xs: Boolean*): Array[Boolean] = { val array = new Array[Boolean](xs.length + 1) array(0) = x @@ -132,6 +135,7 @@ object Array extends FallbackArrayBuilding { } /** Creates an array of `Byte` objects */ + // Subject to a compiler optimization in Cleanup, see above. def apply(x: Byte, xs: Byte*): Array[Byte] = { val array = new Array[Byte](xs.length + 1) array(0) = x @@ -141,6 +145,7 @@ object Array extends FallbackArrayBuilding { } /** Creates an array of `Short` objects */ + // Subject to a compiler optimization in Cleanup, see above. def apply(x: Short, xs: Short*): Array[Short] = { val array = new Array[Short](xs.length + 1) array(0) = x @@ -150,6 +155,7 @@ object Array extends FallbackArrayBuilding { } /** Creates an array of `Char` objects */ + // Subject to a compiler optimization in Cleanup, see above. def apply(x: Char, xs: Char*): Array[Char] = { val array = new Array[Char](xs.length + 1) array(0) = x @@ -159,6 +165,7 @@ object Array extends FallbackArrayBuilding { } /** Creates an array of `Int` objects */ + // Subject to a compiler optimization in Cleanup, see above. def apply(x: Int, xs: Int*): Array[Int] = { val array = new Array[Int](xs.length + 1) array(0) = x @@ -168,6 +175,7 @@ object Array extends FallbackArrayBuilding { } /** Creates an array of `Long` objects */ + // Subject to a compiler optimization in Cleanup, see above. def apply(x: Long, xs: Long*): Array[Long] = { val array = new Array[Long](xs.length + 1) array(0) = x @@ -177,6 +185,7 @@ object Array extends FallbackArrayBuilding { } /** Creates an array of `Float` objects */ + // Subject to a compiler optimization in Cleanup, see above. def apply(x: Float, xs: Float*): Array[Float] = { val array = new Array[Float](xs.length + 1) array(0) = x @@ -186,6 +195,7 @@ object Array extends FallbackArrayBuilding { } /** Creates an array of `Double` objects */ + // Subject to a compiler optimization in Cleanup, see above. def apply(x: Double, xs: Double*): Array[Double] = { val array = new Array[Double](xs.length + 1) array(0) = x diff --git a/src/reflect/scala/reflect/internal/Definitions.scala b/src/reflect/scala/reflect/internal/Definitions.scala index ac1722f069..71559896ab 100644 --- a/src/reflect/scala/reflect/internal/Definitions.scala +++ b/src/reflect/scala/reflect/internal/Definitions.scala @@ -340,12 +340,13 @@ trait Definitions extends api.StandardDefinitions { lazy val PredefModule = requiredModule[scala.Predef.type] lazy val PredefModuleClass = PredefModule.moduleClass - def Predef_classOf = getMemberMethod(PredefModule, nme.classOf) - def Predef_identity = getMemberMethod(PredefModule, nme.identity) - def Predef_conforms = getMemberMethod(PredefModule, nme.conforms) - def Predef_wrapRefArray = getMemberMethod(PredefModule, nme.wrapRefArray) - def Predef_??? = getMemberMethod(PredefModule, nme.???) - def Predef_implicitly = getMemberMethod(PredefModule, nme.implicitly) + def Predef_classOf = getMemberMethod(PredefModule, nme.classOf) + def Predef_identity = getMemberMethod(PredefModule, nme.identity) + def Predef_conforms = getMemberMethod(PredefModule, nme.conforms) + def Predef_wrapRefArray = getMemberMethod(PredefModule, nme.wrapRefArray) + def Predef_wrapArray(tp: Type) = getMemberMethod(PredefModule, wrapArrayMethodName(tp)) + def Predef_??? = getMemberMethod(PredefModule, nme.???) + def Predef_implicitly = getMemberMethod(PredefModule, nme.implicitly) /** Is `sym` a member of Predef with the given name? * Note: DON't replace this by sym == Predef_conforms/etc, as Predef_conforms is a `def` diff --git a/test/files/instrumented/t6611.scala b/test/files/instrumented/t6611.scala index 821d5f3fbf..4c52f8a5ef 100644 --- a/test/files/instrumented/t6611.scala +++ b/test/files/instrumented/t6611.scala @@ -5,7 +5,29 @@ object Test { startProfiling() // tests optimization in Cleanup for varargs reference arrays - val a = Array("") + Array("") + + + Array(true) + Array(true, false) + Array(1: Byte) + Array(1: Byte, 2: Byte) + Array(1: Short) + Array(1: Short, 2: Short) + Array(1) + Array(1, 2) + Array(1L) + Array(1L, 2L) + Array(1d) + Array(1d, 2d) + Array(1f) + Array(1f, 2f) + + /* Not currently optimized: + Array[Int](1, 2) etc + Array(()) + Array((), ()) + */ stopProfiling() printStatistics() diff --git a/test/files/run/t6611.scala b/test/files/run/t6611.scala index c0297372f0..c295368aea 100644 --- a/test/files/run/t6611.scala +++ b/test/files/run/t6611.scala @@ -1,6 +1,61 @@ object Test extends App { - val a = Array("1") - val a2 = Array(a: _*) - a2(0) = "2" - assert(a(0) == "1") + locally { + val a = Array("1") + val a2 = Array(a: _*) + assert(a ne a2) + } + + locally { + val a = Array("1": Object) + val a2 = Array(a: _*) + assert(a ne a2) + } + + locally { + val a = Array(true) + val a2 = Array(a: _*) + assert(a ne a2) + } + + locally { + val a = Array(1: Short) + val a2 = Array(a: _*) + assert(a ne a2) + } + + locally { + val a = Array(1: Byte) + val a2 = Array(a: _*) + assert(a ne a2) + } + + locally { + val a = Array(1) + val a2 = Array(a: _*) + assert(a ne a2) + } + + locally { + val a = Array(1L) + val a2 = Array(a: _*) + assert(a ne a2) + } + + locally { + val a = Array(1f) + val a2 = Array(a: _*) + assert(a ne a2) + } + + locally { + val a = Array(1d) + val a2 = Array(a: _*) + assert(a ne a2) + } + + locally { + val a = Array(()) + val a2 = Array(a: _*) + assert(a ne a2) + } } -- cgit v1.2.3 From 092345a24c22a821204fb358d33272ae8f7353be Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Sun, 4 Nov 2012 14:44:59 +0100 Subject: Refactor guards checking for a particular overload of Array.apply. --- src/compiler/scala/tools/nsc/transform/CleanUp.scala | 11 ++--------- src/reflect/scala/reflect/internal/Definitions.scala | 2 ++ 2 files changed, 4 insertions(+), 9 deletions(-) (limited to 'src/compiler/scala/tools/nsc/transform/CleanUp.scala') diff --git a/src/compiler/scala/tools/nsc/transform/CleanUp.scala b/src/compiler/scala/tools/nsc/transform/CleanUp.scala index 4f145d3d7e..61f2dd39d5 100644 --- a/src/compiler/scala/tools/nsc/transform/CleanUp.scala +++ b/src/compiler/scala/tools/nsc/transform/CleanUp.scala @@ -624,17 +624,10 @@ abstract class CleanUp extends Transform with ast.TreeDSL { // // See SI-6611; we must *only* do this for literal vararg arrays. case Apply(appMeth, List(Apply(wrapRefArrayMeth, List(arg @ StripCast(ArrayValue(_, _)))), _)) - if (wrapRefArrayMeth.symbol == Predef_wrapRefArray && - appMeth.symbol == ArrayModule_overloadedApply.suchThat { - _.tpe.resultType.dealias.typeSymbol == ObjectClass // [T: ClassTag](xs: T*): Array[T] post erasure - }) => + if wrapRefArrayMeth.symbol == Predef_wrapRefArray && appMeth.symbol == ArrayModule_genericApply => super.transform(arg) case Apply(appMeth, List(elem0, Apply(wrapArrayMeth, List(rest @ ArrayValue(elemtpt, _))))) - if wrapArrayMeth.symbol == Predef_wrapArray(elemtpt.tpe) && - appMeth.symbol == ArrayModule_overloadedApply.suchThat { - tp => tp.tpe.paramss.flatten.lift.apply(1).exists(_.tpe.typeSymbol == SeqClass) && - tp.tpe.resultType =:= arrayType(elemtpt.tpe) // (p1: AnyVal1, ps: AnyVal1*): Array[AnyVal1] post erasure - } => + if wrapArrayMeth.symbol == Predef_wrapArray(elemtpt.tpe) && appMeth.symbol == ArrayModule_apply(elemtpt.tpe) => super.transform(rest.copy(elems = elem0 :: rest.elems)) case _ => diff --git a/src/reflect/scala/reflect/internal/Definitions.scala b/src/reflect/scala/reflect/internal/Definitions.scala index 71559896ab..f839df1870 100644 --- a/src/reflect/scala/reflect/internal/Definitions.scala +++ b/src/reflect/scala/reflect/internal/Definitions.scala @@ -471,6 +471,8 @@ trait Definitions extends api.StandardDefinitions { // arrays and their members lazy val ArrayModule = requiredModule[scala.Array.type] lazy val ArrayModule_overloadedApply = getMemberMethod(ArrayModule, nme.apply) + def ArrayModule_genericApply = ArrayModule_overloadedApply.suchThat(_.paramss.flatten.last.tpe.typeSymbol == ClassTagClass) // [T: ClassTag](xs: T*): Array[T] + def ArrayModule_apply(tp: Type) = ArrayModule_overloadedApply.suchThat(_.tpe.resultType =:= arrayType(tp)) // (p1: AnyVal1, ps: AnyVal1*): Array[AnyVal1] lazy val ArrayClass = getRequiredClass("scala.Array") // requiredClass[scala.Array[_]] lazy val Array_apply = getMemberMethod(ArrayClass, nme.apply) lazy val Array_update = getMemberMethod(ArrayClass, nme.update) -- cgit v1.2.3