From 3cbcf07b700a69b1aec7f7573165c21e727b9515 Mon Sep 17 00:00:00 2001 From: Erik Osheim Date: Sat, 28 Sep 2019 17:18:48 -0400 Subject: [PATCH] Respond to Oscar's review comments. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In particular, this adds retrying to the infiniteStream, stringOf, and stringOfN combinators. It also slightly optimizes the loop conditions on buildableOfN, removes lazy from most arbitrary definitions, and cleans up a few other things. This commit does appear to have made some benchmarks slower, although it's possible my machine is just more busy than it was. I've also added a few more benchmarks. Benchmark (genSize) (seedCount) Mode Cnt Score Error Units GenBench.arbitraryString 100 100 avgt 3 600.706 ± 569.014 us/op GenBench.asciiPrintableStr 100 100 avgt 3 432.235 ± 229.533 us/op GenBench.const_ 100 100 avgt 3 2.775 ± 8.017 us/op GenBench.double_ 100 100 avgt 3 9.941 ± 3.020 us/op GenBench.dynamicFrequency 100 100 avgt 3 481.478 ± 253.262 us/op GenBench.eitherIntInt 100 100 avgt 3 30.911 ± 13.071 us/op GenBench.identifier 100 100 avgt 3 186.688 ± 327.920 us/op GenBench.int_ 100 100 avgt 3 11.266 ± 8.500 us/op GenBench.listOfInt 100 100 avgt 3 445.506 ± 403.799 us/op GenBench.mapOfIntInt 100 100 avgt 3 1910.653 ± 2974.722 us/op GenBench.oneOf 100 100 avgt 3 15.945 ± 10.462 us/op GenBench.optionInt 100 100 avgt 3 42.815 ± 18.030 us/op GenBench.sequence 100 100 avgt 3 205.571 ± 42.976 us/op GenBench.staticFrequency 100 100 avgt 3 510.956 ± 111.016 us/op GenBench.testFilter 100 100 avgt 3 1081.890 ± 607.106 us/op GenBench.zipIntInt 100 100 avgt 3 27.987 ± 22.614 us/op --- src/main/scala/org/scalacheck/Arbitrary.scala | 64 ++++++------- src/main/scala/org/scalacheck/Gen.scala | 96 +++++++++++-------- 2 files changed, 90 insertions(+), 70 deletions(-) diff --git a/src/main/scala/org/scalacheck/Arbitrary.scala b/src/main/scala/org/scalacheck/Arbitrary.scala index 77b793bc6..82ac09d2c 100644 --- a/src/main/scala/org/scalacheck/Arbitrary.scala +++ b/src/main/scala/org/scalacheck/Arbitrary.scala @@ -80,29 +80,22 @@ private[scalacheck] sealed trait ArbitraryLowPriority { /**** Arbitrary instances for each AnyVal ****/ - /** Arbitrary AnyVal */ - implicit lazy val arbAnyVal: Arbitrary[AnyVal] = Arbitrary(oneOf( - arbitrary[Unit], arbitrary[Boolean], arbitrary[Char], arbitrary[Byte], - arbitrary[Short], arbitrary[Int], arbitrary[Long], arbitrary[Float], - arbitrary[Double] - )) - /** Arbitrary instance of Boolean */ - implicit lazy val arbBool: Arbitrary[Boolean] = + implicit val arbBool: Arbitrary[Boolean] = Arbitrary(oneOf(true, false)) /** Arbitrary instance of Int */ - implicit lazy val arbInt: Arbitrary[Int] = Arbitrary( + implicit val arbInt: Arbitrary[Int] = Arbitrary( Gen.chooseNum(Int.MinValue, Int.MaxValue) ) /** Arbitrary instance of Long */ - implicit lazy val arbLong: Arbitrary[Long] = Arbitrary( + implicit val arbLong: Arbitrary[Long] = Arbitrary( Gen.chooseNum(Long.MinValue, Long.MaxValue) ) /** Arbitrary instance of Float */ - implicit lazy val arbFloat: Arbitrary[Float] = Arbitrary( + implicit val arbFloat: Arbitrary[Float] = Arbitrary( for { s <- choose(0, 1) e <- choose(0, 0xfe) @@ -111,7 +104,7 @@ private[scalacheck] sealed trait ArbitraryLowPriority { ) /** Arbitrary instance of Double */ - implicit lazy val arbDouble: Arbitrary[Double] = Arbitrary( + implicit val arbDouble: Arbitrary[Double] = Arbitrary( for { s <- choose(0L, 1L) e <- choose(0L, 0x7feL) @@ -120,53 +113,60 @@ private[scalacheck] sealed trait ArbitraryLowPriority { ) /** Arbitrary instance of Char */ - implicit lazy val arbChar: Arbitrary[Char] = + implicit val arbChar: Arbitrary[Char] = Arbitrary(Gen.unicodeChar) /** Arbitrary instance of Byte */ - implicit lazy val arbByte: Arbitrary[Byte] = + implicit val arbByte: Arbitrary[Byte] = Arbitrary(Gen.chooseNum(Byte.MinValue, Byte.MaxValue)) /** Arbitrary instance of Short */ - implicit lazy val arbShort: Arbitrary[Short] = + implicit val arbShort: Arbitrary[Short] = Arbitrary(Gen.chooseNum(Short.MinValue, Short.MaxValue)) /** Absolutely, totally, 100% arbitrarily chosen Unit. */ - implicit lazy val arbUnit: Arbitrary[Unit] = + implicit val arbUnit: Arbitrary[Unit] = Arbitrary(Gen.const(())) + /** Arbitrary AnyVal */ + implicit val arbAnyVal: Arbitrary[AnyVal] = Arbitrary(oneOf( + arbitrary[Unit], arbitrary[Boolean], arbitrary[Char], arbitrary[Byte], + arbitrary[Short], arbitrary[Int], arbitrary[Long], arbitrary[Float], + arbitrary[Double] + )) + /**** Arbitrary instances of other common types ****/ /** Arbitrary instance of String */ - implicit lazy val arbString: Arbitrary[String] = + implicit val arbString: Arbitrary[String] = Arbitrary(Gen.unicodeStr) /** Arbitrary instance of Date */ - implicit lazy val arbDate: Arbitrary[java.util.Date] = + implicit val arbDate: Arbitrary[java.util.Date] = Arbitrary(Gen.calendar.map(_.getTime)) /** Arbitrary instance of Calendar */ - implicit lazy val arbCalendar: Arbitrary[java.util.Calendar] = + implicit val arbCalendar: Arbitrary[java.util.Calendar] = Arbitrary(Gen.calendar) /** Arbitrary instance of Throwable */ - implicit lazy val arbThrowable: Arbitrary[Throwable] = + implicit val arbThrowable: Arbitrary[Throwable] = Arbitrary(oneOf(const(new Exception), const(new Error))) /** Arbitrary instance of Exception */ - implicit lazy val arbException: Arbitrary[Exception] = + implicit val arbException: Arbitrary[Exception] = Arbitrary(const(new Exception)) /** Arbitrary instance of Error */ - implicit lazy val arbError: Arbitrary[Error] = + implicit val arbError: Arbitrary[Error] = Arbitrary(const(new Error)) /** Arbitrary instance of UUID */ - implicit lazy val arbUuid: Arbitrary[java.util.UUID] = + implicit val arbUuid: Arbitrary[java.util.UUID] = Arbitrary(Gen.uuid) /** Arbitrary BigInt */ - implicit lazy val arbBigInt: Arbitrary[BigInt] = { + implicit val arbBigInt: Arbitrary[BigInt] = { val long: Gen[Long] = Gen.choose(Long.MinValue, Long.MaxValue).map(x => if (x == 0) 1L else x) @@ -191,7 +191,7 @@ private[scalacheck] sealed trait ArbitraryLowPriority { } /** Arbitrary BigDecimal */ - implicit lazy val arbBigDecimal: Arbitrary[BigDecimal] = { + implicit val arbBigDecimal: Arbitrary[BigDecimal] = { import java.math.MathContext, MathContext._ val genMathContext0: Gen[MathContext] = @@ -243,7 +243,7 @@ private[scalacheck] sealed trait ArbitraryLowPriority { } /** Arbitrary java.lang.Number */ - implicit lazy val arbNumber: Arbitrary[Number] = { + implicit val arbNumber: Arbitrary[Number] = { val gen = Gen.oneOf( arbitrary[Byte], arbitrary[Short], arbitrary[Int], arbitrary[Long], arbitrary[Float], arbitrary[Double] @@ -254,7 +254,7 @@ private[scalacheck] sealed trait ArbitraryLowPriority { } /** Arbitrary instance of FiniteDuration */ - implicit lazy val arbFiniteDuration: Arbitrary[FiniteDuration] = + implicit val arbFiniteDuration: Arbitrary[FiniteDuration] = Arbitrary(Gen.finiteDuration) /** @@ -263,11 +263,11 @@ private[scalacheck] sealed trait ArbitraryLowPriority { * In addition to `FiniteDuration` values, this can generate `Duration.Inf`, * `Duration.MinusInf`, and `Duration.Undefined`. */ - implicit lazy val arbDuration: Arbitrary[Duration] = + implicit val arbDuration: Arbitrary[Duration] = Arbitrary(Gen.duration) /** Generates an arbitrary property */ - implicit lazy val arbProp: Arbitrary[Prop] = { + implicit val arbProp: Arbitrary[Prop] = { import Prop._ val undecidedOrPassed = forAll { b: Boolean => b ==> true @@ -283,7 +283,7 @@ private[scalacheck] sealed trait ArbitraryLowPriority { } /** Arbitrary instance of test parameters */ - implicit lazy val arbTestParameters: Arbitrary[Test.Parameters] = + implicit val arbTestParameters: Arbitrary[Test.Parameters] = Arbitrary(for { _minSuccTests <- choose(10,200) _maxDiscardRatio <- choose(0.2f,10f) @@ -300,7 +300,7 @@ private[scalacheck] sealed trait ArbitraryLowPriority { ) /** Arbitrary instance of gen params */ - implicit lazy val arbGenParams: Arbitrary[Gen.Parameters] = + implicit val arbGenParams: Arbitrary[Gen.Parameters] = Arbitrary(for { sz <- arbitrary[Int] suchThat (_ >= 0) } yield Gen.Parameters.default.withSize(sz)) @@ -309,7 +309,7 @@ private[scalacheck] sealed trait ArbitraryLowPriority { // Specialised collections // /** Arbitrary instance of scala.collection.BitSet */ - implicit lazy val arbBitSet: Arbitrary[collection.BitSet] = Arbitrary( + implicit val arbBitSet: Arbitrary[collection.BitSet] = Arbitrary( buildableOf[collection.BitSet,Int](sized(sz => choose(0,sz))) ) diff --git a/src/main/scala/org/scalacheck/Gen.scala b/src/main/scala/org/scalacheck/Gen.scala index d1d742e96..abb125f52 100644 --- a/src/main/scala/org/scalacheck/Gen.scala +++ b/src/main/scala/org/scalacheck/Gen.scala @@ -237,7 +237,7 @@ object Gen extends GenArities with GenVersionSpecific { r(None, seed).copy(l = labels) case Some(t) => val r = f(t) - r.copy(l = labels ++ r.labels, sd = r.seed) + r.copy(l = labels | r.labels, sd = r.seed) } } @@ -626,25 +626,27 @@ object Gen extends GenArities with GenVersionSpecific { evb: Buildable[T,C], evt: C => Traversable[T] ): Gen[C] = { require(n >= 0, s"invalid size given: $n") - gen { (p, seed0) => - var seed: Seed = seed0 - val bldr = evb.builder - val allowedFailures = Integer.max(10, n / 10) - var failures = 0 - var count = 0 - while (count < n && failures < allowedFailures) { - val gres = g.doApply(p, seed) - gres.retrieve match { - case Some(t) => - bldr += t - count += 1 - case None => - failures += 1 + new Gen[C] { + def doApply(p: P, seed0: Seed): R[C] = { + var seed: Seed = seed0 + val bldr = evb.builder + val allowedFailures = Gen.collectionRetries(n) + var failures = 0 + var count = 0 + while (count < n) { + val res = g.doApply(p, seed) + res.retrieve match { + case Some(t) => + bldr += t + count += 1 + case None => + failures += 1 + if (failures >= allowedFailures) return r(None, res.seed) + } + seed = res.seed } - seed = gres.seed + r(Some(bldr.result), seed) } - val res = if (count == n) Some(bldr.result) else None - r(res, seed) } } @@ -711,20 +713,27 @@ object Gen extends GenArities with GenVersionSpecific { * is equal to calling containerOfN[Map,(T,U)](n,g). */ def mapOfN[T,U](n: Int, g: Gen[(T, U)]) = buildableOfN[Map[T, U],(T, U)](n, g) - /** Generates an infinite stream. */ + /** + * Generates an infinite stream. + * + * Failures in the underlying generator may terminate the stream. + * Otherwise it will continue forever. + */ def infiniteStream[T](g: => Gen[T]): Gen[Stream[T]] = { - def unfold(p: P, seed: Seed): Stream[T] = { - val r = g.doPureApply(p, seed) - r.retrieve match { - case Some(t) => t #:: unfold(p, r.seed) - case None => Stream.empty + val attemptsPerItem = 10 + def unfold(p: P, seed: Seed, attemptsLeft: Int): Stream[T] = + if (attemptsLeft <= 0) { + Stream.empty + } else { + val r = g.doPureApply(p, seed) + r.retrieve match { + case Some(t) => t #:: unfold(p, r.seed, attemptsPerItem) + case None => unfold(p, r.seed, attemptsLeft - 1) + } } - } gen { (p, seed0) => - new R[Stream[T]] { - val result: Option[Stream[T]] = Some(unfold(p, seed0)) - val seed: Seed = seed0.slide - } + val stream = unfold(p, seed0, attemptsPerItem) + r(Some(stream), seed0.slide) } } @@ -765,7 +774,7 @@ object Gen extends GenArities with GenVersionSpecific { buf += t } else { val (x, s) = seed.long - val i = (x & 0x7fffffff).toInt % count + val i = (x & Long.MaxValue % count).toInt if (i < n) buf(i) = t seed = s } @@ -848,19 +857,25 @@ object Gen extends GenArities with GenVersionSpecific { //// String Generators //// - @tailrec private def mkString(n: Int, sb: StringBuilder, gc: Gen[Char], p: P, seed0: Seed): R[String] = - if (n <= 0) { - r(Some(sb.toString), seed0) - } else { - val res = gc.doApply(p, seed0) + private def mkString(n: Int, sb: StringBuilder, gc: Gen[Char], p: P, seed0: Seed): R[String] = { + var seed: Seed = seed0 + val allowedFailures = Gen.collectionRetries(n) + var failures = 0 + var count = 0 + while (count < n) { + val res = gc.doApply(p, seed) res.retrieve match { case Some(c) => sb += c + count += 1 case None => - () + failures += 1 + if (failures >= allowedFailures) return r(None, res.seed) } - mkString(n - 1, sb, gc, p, res.seed) + seed = res.seed } + r(Some(sb.toString), seed) + } def stringOfN(n: Int, gc: Gen[Char]): Gen[String] = gen { (p, seed) => @@ -1058,9 +1073,14 @@ object Gen extends GenArities with GenVersionSpecific { 1 -> const(Duration.Zero), 6 -> finiteDuration) + // used to compute a uniformly-distributed size private def mkSize(p: Gen.Parameters, seed0: Seed): (Int, Seed) = { val maxSize = Integer.max(p.size + 1, 1) val (x, seed1) = seed0.long - ((x % maxSize).toInt, seed1) + (((x & Long.MaxValue) % maxSize).toInt, seed1) } + + // used to calculate how many per-item retries we should allow. + private def collectionRetries(n: Int): Int = + Integer.max(10, n / 10) }