From de7080281b6fe3a851de9c741cc854da31d1ab22 Mon Sep 17 00:00:00 2001 From: Harrison Houghton Date: Mon, 3 Feb 2020 17:40:50 -0500 Subject: [PATCH 1/6] -Wperformance lints *Ref boxing and nonlocal return -Xlint:nonlocal-return is supported for compatibility. --- .../scala/tools/nsc/settings/Warnings.scala | 19 +++++++++++++++++-- .../tools/nsc/transform/LambdaLift.scala | 3 +++ test/files/neg/t10820-warn.scala | 2 +- test/files/neg/xlint-captured.check | 9 +++++++++ test/files/neg/xlint-captured.scala | 11 +++++++++++ test/files/run/nonlocalreturn.check | 1 - test/files/run/nonlocalreturn.scala | 4 +--- test/files/run/retclosure.check | 1 - test/files/run/retclosure.scala | 4 +--- test/files/run/spec-nlreturn.check | 2 -- test/files/run/spec-nlreturn.scala | 16 ++++++---------- 11 files changed, 49 insertions(+), 23 deletions(-) create mode 100644 test/files/neg/xlint-captured.check create mode 100644 test/files/neg/xlint-captured.scala delete mode 100644 test/files/run/nonlocalreturn.check delete mode 100644 test/files/run/retclosure.check delete mode 100644 test/files/run/spec-nlreturn.check diff --git a/src/compiler/scala/tools/nsc/settings/Warnings.scala b/src/compiler/scala/tools/nsc/settings/Warnings.scala index 9dbe262b4e81..f8194a0ffb67 100644 --- a/src/compiler/scala/tools/nsc/settings/Warnings.scala +++ b/src/compiler/scala/tools/nsc/settings/Warnings.scala @@ -98,7 +98,7 @@ trait Warnings { |to prevent the shell from expanding patterns.""".stripMargin), prepend = true) - // Non-lint warnings. -- TODO turn into MultiChoiceEnumeration + // Non-lint warnings. val warnMacros = ChoiceSetting( name = "-Wmacros", helpArg = "mode", @@ -117,6 +117,20 @@ trait Warnings { val warnNumericWiden = BooleanSetting("-Wnumeric-widen", "Warn when numerics are widened.") withAbbreviation "-Ywarn-numeric-widen" val warnOctalLiteral = BooleanSetting("-Woctal-literal", "Warn on obsolete octal syntax.") withAbbreviation "-Ywarn-octal-literal" + object PerformanceWarnings extends MultiChoiceEnumeration { + val Captured = Choice("captured", "Modification of var in closure causes boxing.") + val NonlocalReturn = Choice("nonlocal-return", "A return statement used an exception for flow control.") + } + val warnPerformance = MultiChoiceSetting( + name = "-Wperformance", + helpArg = "warning", + descr = "Enable or disable specific lints for performance", + domain = PerformanceWarnings, + default = Some(List("_")) + ) + def warnCaptured = warnPerformance.contains(PerformanceWarnings.Captured) + def warnNonlocalReturn = warnPerformance.contains(PerformanceWarnings.NonlocalReturn) + object UnusedWarnings extends MultiChoiceEnumeration { val Imports = Choice("imports", "Warn if an import selector is not referenced.") val PatVars = Choice("patvars", "Warn if a variable bound in a pattern is unused.") @@ -215,7 +229,6 @@ trait Warnings { def warnStarsAlign = lint contains StarsAlign def warnConstant = lint contains Constant def lintUnused = lint contains Unused - def warnNonlocalReturn = lint contains NonlocalReturn def lintImplicitNotFound = lint contains ImplicitNotFound def warnSerialization = lint contains Serial def lintValPatterns = lint contains ValPattern @@ -239,6 +252,8 @@ trait Warnings { if (s contains Unused) warnUnused.enable(UnusedWarnings.Linted) else warnUnused.disable(UnusedWarnings.Linted) if (s.contains(Deprecation) && deprecation.isDefault) deprecation.value = true + if (s.contains(NonlocalReturn)) warnPerformance.enable(PerformanceWarnings.NonlocalReturn) + else warnPerformance.disable(PerformanceWarnings.NonlocalReturn) } // Backward compatibility. diff --git a/src/compiler/scala/tools/nsc/transform/LambdaLift.scala b/src/compiler/scala/tools/nsc/transform/LambdaLift.scala index 4de4fe1c7b12..53c4870a5ad7 100644 --- a/src/compiler/scala/tools/nsc/transform/LambdaLift.scala +++ b/src/compiler/scala/tools/nsc/transform/LambdaLift.scala @@ -505,6 +505,9 @@ abstract class LambdaLift extends InfoTransform { } } + if (settings.warnCaptured) + reporter.warning(tree.pos, s"Modification of variable $name within a closure causes it to be boxed.") + treeCopy.ValDef(tree, mods, name, tpt1, factoryCall) } else tree case Return(Block(stats, value)) => diff --git a/test/files/neg/t10820-warn.scala b/test/files/neg/t10820-warn.scala index a04f62a5443d..97796aaa90d1 100644 --- a/test/files/neg/t10820-warn.scala +++ b/test/files/neg/t10820-warn.scala @@ -1,4 +1,4 @@ -// scalac: -Xfatal-warnings -Xlint:nonlocal-return +// scalac: -Werror -Wperformance // import util.Try diff --git a/test/files/neg/xlint-captured.check b/test/files/neg/xlint-captured.check new file mode 100644 index 000000000000..36584ac470c3 --- /dev/null +++ b/test/files/neg/xlint-captured.check @@ -0,0 +1,9 @@ +xlint-captured.scala:10: warning: return statement uses an exception to pass control to the caller of the enclosing named method f + def f(): Unit = List(42).foreach(i => if (i > 27) return) + ^ +xlint-captured.scala:5: warning: Modification of variable c within a closure causes it to be boxed. + var c = a // nok + ^ +error: No warnings can be incurred under -Werror. +2 warnings +1 error diff --git a/test/files/neg/xlint-captured.scala b/test/files/neg/xlint-captured.scala new file mode 100644 index 000000000000..e4acdf7bcc59 --- /dev/null +++ b/test/files/neg/xlint-captured.scala @@ -0,0 +1,11 @@ +// scalac: -Werror -Wperformance +object Test { + var a, b = 0 // ok + def mkStrangeCounter(): Int => Int = { + var c = a // nok + object _d { var d = b }; import _d._ // ok + e => { c += a; d += b; a *= b; b -= c; c ^ d } + } + + def f(): Unit = List(42).foreach(i => if (i > 27) return) +} diff --git a/test/files/run/nonlocalreturn.check b/test/files/run/nonlocalreturn.check deleted file mode 100644 index aeb2d5e2398d..000000000000 --- a/test/files/run/nonlocalreturn.check +++ /dev/null @@ -1 +0,0 @@ -Some(1) diff --git a/test/files/run/nonlocalreturn.scala b/test/files/run/nonlocalreturn.scala index 399cb0709c6e..0bc4a8d29a85 100644 --- a/test/files/run/nonlocalreturn.scala +++ b/test/files/run/nonlocalreturn.scala @@ -6,9 +6,7 @@ object Test { wrap({ return Some(1) ; None }) } - def main(args: Array[String]): Unit = { - println(f()) - } + def main(args: Array[String]): Unit = assert(f() == Some(1)) } // java.lang.ClassCastException: scala.Some cannot be cast to scala.None$ // at Test$$anonfun$f$1.apply(nonlocalreturn.scala:5) diff --git a/test/files/run/retclosure.check b/test/files/run/retclosure.check deleted file mode 100644 index 94c4971e4a42..000000000000 --- a/test/files/run/retclosure.check +++ /dev/null @@ -1 +0,0 @@ -check failed: some problem diff --git a/test/files/run/retclosure.scala b/test/files/run/retclosure.scala index 18d2aa937727..46e35074141f 100644 --- a/test/files/run/retclosure.scala +++ b/test/files/run/retclosure.scala @@ -17,7 +17,5 @@ object Test { } } - def main(args: Array[String]): Unit = { - Console.println(response) - } + def main(args: Array[String]): Unit = assert(response == "check failed: some problem") } diff --git a/test/files/run/spec-nlreturn.check b/test/files/run/spec-nlreturn.check deleted file mode 100644 index 26cff0736032..000000000000 --- a/test/files/run/spec-nlreturn.check +++ /dev/null @@ -1,2 +0,0 @@ -scala.runtime.NonLocalReturnControl$mcI$sp -16 diff --git a/test/files/run/spec-nlreturn.scala b/test/files/run/spec-nlreturn.scala index 8299901507d8..d9238a67863a 100644 --- a/test/files/run/spec-nlreturn.scala +++ b/test/files/run/spec-nlreturn.scala @@ -1,17 +1,13 @@ //scalac: -Xlint:-nonlocal-return object Test { def f(): Int = { - try { - val g = (1 to 10 map { i => return 16 ; i }).sum - g - } - catch { case x: runtime.NonLocalReturnControl[_] => - println(x.getClass.getName) - x.value.asInstanceOf[Int] + try (1 to 10).map { i => return 16 ; i }.sum + catch { + case x: runtime.NonLocalReturnControl[_] => + assert(x.getClass.getName == "scala.runtime.NonLocalReturnControl$mcI$sp") + x.value.asInstanceOf[Int] } } - def main(args: Array[String]): Unit = { - println(f()) - } + def main(args: Array[String]): Unit = assert(f() == 16) } From 5e008903eb7ac8989f13eb6f792ad37938ff09cd Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Sun, 2 Jan 2022 15:32:03 -0800 Subject: [PATCH 2/6] Possible micro-optimizations --- src/library/scala/StringContext.scala | 43 ++++----------- src/library/scala/collection/ArrayOps.scala | 35 +++++------- src/library/scala/collection/Iterable.scala | 25 +++++---- .../scala/collection/IterableOnce.scala | 53 ++++++++++--------- test/files/run/scan.scala | 2 +- .../junit/scala/collection/ArrayOpsTest.scala | 13 ++++- 6 files changed, 82 insertions(+), 89 deletions(-) diff --git a/src/library/scala/StringContext.scala b/src/library/scala/StringContext.scala index e9684b832ee5..4358efa1122b 100644 --- a/src/library/scala/StringContext.scala +++ b/src/library/scala/StringContext.scala @@ -14,6 +14,7 @@ package scala import java.lang.{ StringBuilder => JLSBuilder } import scala.annotation.tailrec +import scala.collection.mutable.ArrayBuilder /** This class provides the basic mechanism to do String Interpolation. * String Interpolation allows users @@ -219,32 +220,15 @@ object StringContext { val nameLength = input.length // The final pattern is as long as all the chunks, separated by 1-character // glob-wildcard placeholders - val patternLength = { - var n = numWildcards - for(chunk <- patternChunks) { - n += chunk.length - } - n - } + val patternLength = patternChunks.iterator.map(_.length).sum + numWildcards // Convert the input pattern chunks into a single sequence of shorts; each // non-negative short represents a character, while -1 represents a glob wildcard val pattern = { - val arr = new Array[Short](patternLength) - var i = 0 - var first = true - for(chunk <- patternChunks) { - if (first) first = false - else { - arr(i) = -1 - i += 1 - } - for(c <- chunk) { - arr(i) = c.toShort - i += 1 - } - } - arr + val b = new ArrayBuilder.ofShort ; b.sizeHint(patternLength) + patternChunks.head.foreach(c => b.addOne(c.toShort)) + patternChunks.tail.foreach { s => b.addOne(-1) ; s.foreach(c => b.addOne(c.toShort)) } + b.result() } // Lookup table for each character in the pattern to check whether or not @@ -252,20 +236,15 @@ object StringContext { // glob wildcard it represents, while -1 means it doesn't represent any val matchIndices = { val arr = Array.fill(patternLength + 1)(-1) - var i = 0 - var j = 0 - for(chunk <- patternChunks) { - if (j < numWildcards) { - i += chunk.length - arr(i) = j - i += 1 - j += 1 - } + patternChunks.init.zipWithIndex.foldLeft(0) { case (ttl, (chunk, i)) => + val sum = ttl + chunk.length + arr(sum) = i + sum + 1 } arr } - while(patternIndex < patternLength || inputIndex < nameLength) { + while (patternIndex < patternLength || inputIndex < nameLength) { matchIndices(patternIndex) match { case -1 => // do nothing case n => diff --git a/src/library/scala/collection/ArrayOps.scala b/src/library/scala/collection/ArrayOps.scala index a4948ac01f2e..ae1119cb8494 100644 --- a/src/library/scala/collection/ArrayOps.scala +++ b/src/library/scala/collection/ArrayOps.scala @@ -173,6 +173,9 @@ object ArrayOps { * an implementation that copies the data to a boxed representation for use with `Arrays.sort`. */ private final val MaxStableSortLength = 300 + + /** Avoid an allocation in [[collect]]. */ + private val fallback: Any => Any = _ => fallback } /** This class serves as a wrapper for `Array`s with many of the operations found in @@ -1010,18 +1013,13 @@ final class ArrayOps[A](private val xs: Array[A]) extends AnyVal { * `pf` to each element on which it is defined and collecting the results. * The order of the elements is preserved. */ - def collect[B : ClassTag](pf: PartialFunction[A, B]): Array[B] = { - var i = 0 - var matched = true - def d(x: A): B = { - matched = false - null.asInstanceOf[B] - } + def collect[B: ClassTag](pf: PartialFunction[A, B]): Array[B] = { + val fallback: Any => Any = ArrayOps.fallback val b = ArrayBuilder.make[B] - while(i < xs.length) { - matched = true - val v = pf.applyOrElse(xs(i), d) - if(matched) b += v + var i = 0 + while (i < xs.length) { + val v = pf.applyOrElse(xs(i), fallback) + if (v.asInstanceOf[AnyRef] ne fallback) b.addOne(v.asInstanceOf[B]) i += 1 } b.result() @@ -1029,17 +1027,12 @@ final class ArrayOps[A](private val xs: Array[A]) extends AnyVal { /** Finds the first element of the array for which the given partial function is defined, and applies the * partial function to it. */ - def collectFirst[B](f: PartialFunction[A, B]): Option[B] = { + def collectFirst[B](@deprecatedName("f","2.13.9") pf: PartialFunction[A, B]): Option[B] = { + val fallback: Any => Any = ArrayOps.fallback var i = 0 - var matched = true - def d(x: A): B = { - matched = false - null.asInstanceOf[B] - } - while(i < xs.length) { - matched = true - val v = f.applyOrElse(xs(i), d) - if(matched) return Some(v) + while (i < xs.length) { + val v = pf.applyOrElse(xs(i), fallback) + if (v.asInstanceOf[AnyRef] ne fallback) return Some(v.asInstanceOf[B]) i += 1 } None diff --git a/src/library/scala/collection/Iterable.scala b/src/library/scala/collection/Iterable.scala index db4f7b919943..692a244f9f9e 100644 --- a/src/library/scala/collection/Iterable.scala +++ b/src/library/scala/collection/Iterable.scala @@ -597,11 +597,13 @@ trait IterableOps[+A, +CC[_], +C] extends Any with IterableOnce[A] with Iterable val bldr = m.getOrElseUpdate(k, iterableFactory.newBuilder[B]) bldr += f(elem) } - var result = immutable.Map.empty[K, CC[B]] - m.foreach { case (k, v) => - result = result + ((k, v.result())) + object result extends Function[(K, Builder[B, CC[B]]), Unit] { + var built = immutable.Map.empty[K, CC[B]] + def apply(kv: (K, Builder[B, CC[B]])) = + built = built.updated(kv._1, kv._2.result()) } - result + m.foreach(result) + result.built } /** @@ -663,13 +665,16 @@ trait IterableOps[+A, +CC[_], +C] extends Any with IterableOnce[A] with Iterable * @return collection with intermediate results */ def scanRight[B](z: B)(op: (A, B) => B): CC[B] = { - var scanned = z :: immutable.Nil - var acc = z - for (x <- reversed) { - acc = op(x, acc) - scanned ::= acc + object scanner extends (A => Unit) { + var acc = z + var scanned = acc :: immutable.Nil + def apply(x: A) = { + acc = op(x, acc) + scanned ::= acc + } } - iterableFactory.from(scanned) + reversed.foreach(scanner) + iterableFactory.from(scanner.scanned) } def map[B](f: A => B): CC[B] = iterableFactory.from(new View.Map(this, f)) diff --git a/src/library/scala/collection/IterableOnce.scala b/src/library/scala/collection/IterableOnce.scala index a9ab03a00117..f52802ca0a20 100644 --- a/src/library/scala/collection/IterableOnce.scala +++ b/src/library/scala/collection/IterableOnce.scala @@ -501,8 +501,11 @@ trait IterableOnceOps[+A, +CC[_], +C] extends Any { this: IterableOnce[A] => * elements of this $coll, and the other elements. */ def splitAt(n: Int): (C, C) = { - var i = 0 - span { _ => if (i < n) { i += 1; true } else false } + object spanner extends (A => Boolean) { + var i = 0 + def apply(a: A) = i < n && { i += 1 ; true } + } + span(spanner) } /** Applies a side-effecting function to each element in this collection. @@ -977,19 +980,20 @@ trait IterableOnceOps[+A, +CC[_], +C] extends Any { this: IterableOnce[A] => if (isEmpty) throw new UnsupportedOperationException("empty.maxBy") - var maxF: B = null.asInstanceOf[B] - var maxElem: A = null.asInstanceOf[A] - var first = true - - for (elem <- this) { - val fx = f(elem) - if (first || cmp.gt(fx, maxF)) { - maxElem = elem - maxF = fx - first = false + object maximizer extends (A => Unit) { + var maxF: B = null.asInstanceOf[B] + var maxElem: A = null.asInstanceOf[A] + var first = true + def apply(elem: A) = { + val fx = f(elem) + if (first && { first = false ; true } || cmp.gt(fx, maxF)) { + maxElem = elem + maxF = fx + } } } - maxElem + foreach(maximizer) + maximizer.maxElem } /** Finds the first element which yields the largest value measured by function f. @@ -1024,19 +1028,20 @@ trait IterableOnceOps[+A, +CC[_], +C] extends Any { this: IterableOnce[A] => if (isEmpty) throw new UnsupportedOperationException("empty.minBy") - var minF: B = null.asInstanceOf[B] - var minElem: A = null.asInstanceOf[A] - var first = true - - for (elem <- this) { - val fx = f(elem) - if (first || cmp.lt(fx, minF)) { - minElem = elem - minF = fx - first = false + object minimizer extends (A => Unit) { + var minF: B = null.asInstanceOf[B] + var minElem: A = null.asInstanceOf[A] + var first = true + def apply(elem: A) = { + val fx = f(elem) + if (first && { first = false ; true } || cmp.lt(fx, minF)) { + minElem = elem + minF = fx + } } } - minElem + foreach(minimizer) + minimizer.minElem } /** Finds the first element which yields the smallest value measured by function f. diff --git a/test/files/run/scan.scala b/test/files/run/scan.scala index d6708d325fd2..b52dde0afb7b 100644 --- a/test/files/run/scan.scala +++ b/test/files/run/scan.scala @@ -4,7 +4,7 @@ object Test { val lst = List(1, 2, 3, 4, 5) assert(lst.scanLeft(0)(_ + _) == List(0, 1, 3, 6, 10, 15)) - assert(lst.scanRight(0)(_ + _) == List(15, 14, 12, 9, 5, 0)) + assert(lst.scanRight(0)(_ + _) == List(15, 14, 12, 9, 5, 0), "List scanRight") val emp = List[Int]() assert(emp.scanLeft(0)(_ + _) == List(0)) diff --git a/test/junit/scala/collection/ArrayOpsTest.scala b/test/junit/scala/collection/ArrayOpsTest.scala index d8f9eb4229f3..b2aaf4d5c0c1 100644 --- a/test/junit/scala/collection/ArrayOpsTest.scala +++ b/test/junit/scala/collection/ArrayOpsTest.scala @@ -150,8 +150,19 @@ class ArrayOpsTest { .iterator .drop(Int.MaxValue) .drop(Int.MaxValue) // potential index overflow to negative - assert(!it.hasNext) // bug had index as negative and this returning true + assertFalse(it.hasNext)// bug had index as negative and this returning true // even though the index is both out of bounds and should // always be between `0` and `Array#length`. } + + @Test def `array collect`: Unit = { + val vs = (1 to 10).toArray + val res = vs.collect { case n if n%2 == 0 => 10*n } + assertArrayEquals((2 to 10 by 2).map(_ * 10).toArray, res) + } + @Test def `array collect first`: Unit = { + val vs = (1 to 10).toArray + val res = vs.collectFirst { case n if n > 4 => 10*n } + assertEquals(Some(50), res) + } } From 51d29c7680a69074e29503f82b1bd45c80efe89f Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Sun, 2 Jan 2022 19:21:22 -0800 Subject: [PATCH 3/6] StringOps.collect --- src/library/scala/collection/StringOps.scala | 29 ++++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/src/library/scala/collection/StringOps.scala b/src/library/scala/collection/StringOps.scala index 42a06f6e7ce4..2f49926aaa05 100644 --- a/src/library/scala/collection/StringOps.scala +++ b/src/library/scala/collection/StringOps.scala @@ -155,6 +155,9 @@ object StringOps { /** Creates a new non-strict filter which combines this filter with the given predicate. */ def withFilter(q: Char => Boolean): WithFilter = new WithFilter(a => p(a) && q(a), s) } + + /** Avoid an allocation in [[collect]]. */ + private val fallback: Any => Any = _ => fallback } /** Provides extension methods for strings. @@ -270,17 +273,12 @@ final class StringOps(private val s: String) extends AnyVal { * `pf` to each char on which it is defined and collecting the results. */ def collect(pf: PartialFunction[Char, Char]): String = { + val fallback: Any => Any = StringOps.fallback var i = 0 - var matched = true - def d(x: Char): Char = { - matched = false - 0 - } val b = new StringBuilder - while(i < s.length) { - matched = true - val v = pf.applyOrElse(s.charAt(i), d) - if(matched) b += v + while (i < s.length) { + val v = pf.applyOrElse(s.charAt(i), fallback) + if (v.asInstanceOf[AnyRef] ne fallback) b.addOne(v.asInstanceOf[Char]) i += 1 } b.result() @@ -295,17 +293,12 @@ final class StringOps(private val s: String) extends AnyVal { * `pf` to each char on which it is defined and collecting the results. */ def collect[B](pf: PartialFunction[Char, B]): immutable.IndexedSeq[B] = { + val fallback: Any => Any = StringOps.fallback var i = 0 - var matched = true - def d(x: Char): B = { - matched = false - null.asInstanceOf[B] - } val b = immutable.IndexedSeq.newBuilder[B] - while(i < s.length) { - matched = true - val v = pf.applyOrElse(s.charAt(i), d) - if(matched) b += v + while (i < s.length) { + val v = pf.applyOrElse(s.charAt(i), fallback) + if (v.asInstanceOf[AnyRef] ne fallback) b.addOne(v.asInstanceOf[B]) i += 1 } b.result() From c2aa706a6871ca6a24b9085125c5750f3834229c Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Sun, 2 Jan 2022 19:21:44 -0800 Subject: [PATCH 4/6] GroupMap benchmark --- .../scala/collection/GroupMapBenchmarks.scala | 182 ++++++++++++++++++ 1 file changed, 182 insertions(+) create mode 100755 test/benchmarks/src/main/scala/scala/collection/GroupMapBenchmarks.scala diff --git a/test/benchmarks/src/main/scala/scala/collection/GroupMapBenchmarks.scala b/test/benchmarks/src/main/scala/scala/collection/GroupMapBenchmarks.scala new file mode 100755 index 000000000000..977946c991eb --- /dev/null +++ b/test/benchmarks/src/main/scala/scala/collection/GroupMapBenchmarks.scala @@ -0,0 +1,182 @@ +package scala.collection + +import java.util.concurrent.TimeUnit + +import org.openjdk.jmh.annotations._ +import org.openjdk.jmh.infra.Blackhole + + +@BenchmarkMode(scala.Array(Mode.AverageTime)) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Fork(1) +@Warmup(iterations = 8) +@Measurement(iterations = 8) +@State(Scope.Benchmark) +class GroupMapBenchmark { + + type CC[A] = immutable.List[A] + val factory = immutable.List + + @Param(scala.Array("2", "3", "5", "16", "17", "32", "33", "128", "129")) + var size: Int = _ + + var xs: CC[Long] = _ + + def fresh(n: Int) = factory((1 to n).map(_.toLong): _*) + + @Setup(Level.Trial) + def initTrial(): Unit = { + xs = fresh(10000) + } + + @Benchmark + def groupMap(bh: Blackhole): Unit = + xs.groupMap(_ % size)(_ * 2).foreach(bh.consume) + +} + +@BenchmarkMode(scala.Array(Mode.AverageTime)) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Fork(1) +@Warmup(iterations = 8) +@Measurement(iterations = 8) +@State(Scope.Benchmark) +class GroupMapValuesBenchmark { + + type CC[A] = immutable.List[A] + val factory = immutable.List + + @Param(scala.Array("2", "3", "5", "16", "17", "32", "33", "128", "129")) + var size: Int = _ + + var xs: CC[Long] = _ + + def fresh(n: Int) = factory((1 to n).map(_.toLong): _*) + + @Setup(Level.Trial) + def initTrial(): Unit = { + xs = fresh(10000) + } + + @Benchmark + def groupMap(bh: Blackhole): Unit = + xs.groupBy(_ % size).mapValues(_.map(_ * 2)).foreach(bh.consume) + +} + +@BenchmarkMode(scala.Array(Mode.AverageTime)) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Fork(1) +@Warmup(iterations = 8) +@Measurement(iterations = 8) +@State(Scope.Benchmark) +class ScalaGroupMapValuesBenchmark { + + type CC[A] = scala.collection.immutable.List[A] + val factory = scala.collection.immutable.List + + @Param(scala.Array("2", "3", "5", "16", "17", "32", "33", "128", "129")) + var size: Int = _ + + var xs: CC[Long] = _ + + def fresh(n: Int) = factory((1 to n).map(_.toLong): _*) + + @Setup(Level.Trial) + def initTrial(): Unit = { + xs = fresh(10000) + } + + @Benchmark + def groupMap(bh: Blackhole): Unit = + xs.groupBy(_ % size).mapValues(_.map(_ * 2)).foreach(bh.consume) + +} + +@BenchmarkMode(scala.Array(Mode.AverageTime)) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Fork(1) +@Warmup(iterations = 8) +@Measurement(iterations = 8) +@State(Scope.Benchmark) +class GroupMapReduceBenchmark { + + type CC[A] = immutable.List[A] + val factory = immutable.List + + @Param(scala.Array("2", "3", "5", "16", "17", "32", "33", "128", "129")) + var size: Int = _ + + var xs: CC[Long] = _ + + def fresh(n: Int) = factory((1 to n).map(_.toLong): _*) + + @Setup(Level.Trial) + def initTrial(): Unit = { + xs = fresh(10000) + } + + @Benchmark + def groupMapReduce(bh: Blackhole): Unit = + xs.groupMapReduce(_ % size)(_ * 2)(_ + _).foreach(bh.consume) + +} + +@BenchmarkMode(scala.Array(Mode.AverageTime)) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Fork(1) +@Warmup(iterations = 8) +@Measurement(iterations = 8) +@State(Scope.Benchmark) +class GroupMapValuesReduceBenchmark { + + type CC[A] = immutable.List[A] + val factory = immutable.List + + @Param(scala.Array("2", "3", "5", "16", "17", "32", "33", "128", "129")) + var size: Int = _ + + var xs: CC[Long] = _ + + def fresh(n: Int) = factory((1 to n).map(_.toLong): _*) + + @Setup(Level.Trial) + def initTrial(): Unit = { + xs = fresh(10000) + } + + @Benchmark + def groupMapReduce(bh: Blackhole): Unit = + xs.groupBy(_ % size).mapValues(_.map(_ * 2).reduce(_ + _)).foreach(bh.consume) + +} + + +@BenchmarkMode(scala.Array(Mode.AverageTime)) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Fork(1) +@Warmup(iterations = 8) +@Measurement(iterations = 8) +@State(Scope.Benchmark) +class ScalaGroupMapValuesReduceBenchmark { + + type CC[A] = scala.collection.immutable.List[A] + val factory = scala.collection.immutable.List + + @Param(scala.Array("2", "3", "5", "16", "17", "32", "33", "128", "129")) + var size: Int = _ + + var xs: CC[Long] = _ + + def fresh(n: Int) = factory((1 to n).map(_.toLong): _*) + + @Setup(Level.Trial) + def initTrial(): Unit = { + xs = fresh(10000) + } + + @Benchmark + def groupMapReduce(bh: Blackhole): Unit = + xs.groupBy(_ % size).mapValues(_.map(_ * 2).reduce(_ + _)).foreach(bh.consume) + +} From b95d5116a10747b0ff7f40bad34e8e97f4322d14 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Sun, 2 Jan 2022 19:22:03 -0800 Subject: [PATCH 5/6] LamdaLift tweak --- .../tools/nsc/transform/LambdaLift.scala | 37 ++++++++----------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/src/compiler/scala/tools/nsc/transform/LambdaLift.scala b/src/compiler/scala/tools/nsc/transform/LambdaLift.scala index 53c4870a5ad7..e4a8a10645e1 100644 --- a/src/compiler/scala/tools/nsc/transform/LambdaLift.scala +++ b/src/compiler/scala/tools/nsc/transform/LambdaLift.scala @@ -488,28 +488,23 @@ abstract class LambdaLift extends InfoTransform { if (sym.isLocalToBlock) liftDef(withFreeParams) else withFreeParams - case ValDef(mods, name, tpt, rhs) => - if (sym.isCapturedVariable) { - val tpt1 = TypeTree(sym.tpe) setPos tpt.pos - - val refTypeSym = sym.tpe.typeSymbol - - val factoryCall = typer.typedPos(rhs.pos) { - rhs match { - case EmptyTree => - val zeroMSym = refZeroMethod(refTypeSym) - gen.mkMethodCall(zeroMSym, Nil) - case arg => - val createMSym = refCreateMethod(refTypeSym) - gen.mkMethodCall(createMSym, arg :: Nil) - } + case ValDef(mods, name, tpt, rhs) if sym.isCapturedVariable => + val tpt1 = TypeTree(sym.tpe) setPos tpt.pos + val refTypeSym = sym.tpe.typeSymbol + val factoryCall = typer.typedPos(rhs.pos) { + rhs match { + case EmptyTree => + val zeroMSym = refZeroMethod(refTypeSym) + gen.mkMethodCall(zeroMSym, Nil) + case arg => + val createMSym = refCreateMethod(refTypeSym) + gen.mkMethodCall(createMSym, arg :: Nil) } - - if (settings.warnCaptured) - reporter.warning(tree.pos, s"Modification of variable $name within a closure causes it to be boxed.") - - treeCopy.ValDef(tree, mods, name, tpt1, factoryCall) - } else tree + } + if (settings.warnCaptured) + reporter.warning(tree.pos, s"Modification of variable $name within a closure causes it to be boxed.") + treeCopy.ValDef(tree, mods, name, tpt1, factoryCall) + case ValDef(_, _, _, _) => tree case Return(Block(stats, value)) => Block(stats, treeCopy.Return(tree, value)) setType tree.tpe setPos tree.pos case Return(expr) => From 6654dcfc47f34c8a5694b255aa2f4b7b2baad2f3 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Fri, 4 Mar 2022 05:14:28 -0800 Subject: [PATCH 6/6] Use runReporting lint-performance --- src/compiler/scala/tools/nsc/Reporting.scala | 1 + .../scala/tools/nsc/transform/LambdaLift.scala | 3 ++- test/files/pos/ignore-wperf.scala | 11 +++++++++++ 3 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 test/files/pos/ignore-wperf.scala diff --git a/src/compiler/scala/tools/nsc/Reporting.scala b/src/compiler/scala/tools/nsc/Reporting.scala index f113a3789ad2..9a27236817e9 100644 --- a/src/compiler/scala/tools/nsc/Reporting.scala +++ b/src/compiler/scala/tools/nsc/Reporting.scala @@ -395,6 +395,7 @@ object Reporting { object LintRecurseWithDefault extends Lint; add(LintRecurseWithDefault) object LintUnitSpecialization extends Lint; add(LintUnitSpecialization) object LintMultiargInfix extends Lint; add(LintMultiargInfix) + object LintPerformance extends Lint; add(LintPerformance) sealed trait Feature extends WarningCategory { override def summaryCategory: WarningCategory = Feature } object Feature extends Feature { override def includes(o: WarningCategory): Boolean = o.isInstanceOf[Feature] }; add(Feature) diff --git a/src/compiler/scala/tools/nsc/transform/LambdaLift.scala b/src/compiler/scala/tools/nsc/transform/LambdaLift.scala index e4a8a10645e1..11068d129388 100644 --- a/src/compiler/scala/tools/nsc/transform/LambdaLift.scala +++ b/src/compiler/scala/tools/nsc/transform/LambdaLift.scala @@ -18,6 +18,7 @@ import Flags._ import scala.annotation.tailrec import scala.collection.mutable import scala.collection.mutable.{LinkedHashMap, LinkedHashSet} +import scala.tools.nsc.Reporting.WarningCategory.LintPerformance abstract class LambdaLift extends InfoTransform { import global._ @@ -502,7 +503,7 @@ abstract class LambdaLift extends InfoTransform { } } if (settings.warnCaptured) - reporter.warning(tree.pos, s"Modification of variable $name within a closure causes it to be boxed.") + runReporting.warning(tree.pos, s"Modification of variable $name within a closure causes it to be boxed.", LintPerformance, sym) treeCopy.ValDef(tree, mods, name, tpt1, factoryCall) case ValDef(_, _, _, _) => tree case Return(Block(stats, value)) => diff --git a/test/files/pos/ignore-wperf.scala b/test/files/pos/ignore-wperf.scala new file mode 100644 index 000000000000..24c4e6a5c4ba --- /dev/null +++ b/test/files/pos/ignore-wperf.scala @@ -0,0 +1,11 @@ +// scalac: -Werror -Wperformance -Wconf:cat=lint-performance:s + +class C { + var x = 0 + + def f: (Int => Int) = { + var y = x + z => y + 1 + } +} +//test/files/neg/ignore-wperf.scala:7: warning: Modification of variable y within a closure causes it to be boxed.