From f331cc8471e6495080b2037cefb4f874ea4fa3c6 Mon Sep 17 00:00:00 2001 From: Ben Plommer Date: Thu, 7 Apr 2022 19:55:58 +0100 Subject: [PATCH 1/7] Optimize Chain.length implementation --- core/src/main/scala/cats/data/Chain.scala | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/core/src/main/scala/cats/data/Chain.scala b/core/src/main/scala/cats/data/Chain.scala index 1b8d4f7229..66b8695197 100644 --- a/core/src/main/scala/cats/data/Chain.scala +++ b/core/src/main/scala/cats/data/Chain.scala @@ -565,16 +565,15 @@ sealed abstract class Chain[+A] { /** * Returns the number of elements in this structure */ - final def length: Long = { - // TODO: consider optimizing for `Chain.Wrap` case. - // Some underlying seq may not need enumerating all elements to calculate its size. - val iter = iterator - var i: Long = 0 - while (iter.hasNext) { i += 1; iter.next(); } - i - } + final def length: Long = + this match { + case Empty => 0 + case Wrap(seq) => seq.length + case Singleton(a) => 1 + case Append(leftNE, rightNE) => leftNE.length + rightNE.length + } - /** + /* * Alias for length */ final def size: Long = length From 4ec151c8e7fbb7a262abcef263796b868d29cdae Mon Sep 17 00:00:00 2001 From: Ben Plommer Date: Thu, 7 Apr 2022 20:05:45 +0100 Subject: [PATCH 2/7] Optimize Chain.knownLength --- .../scala-2.12/cats/compat/ChainCompat.scala | 15 +++++++++++++++ .../scala-2.13+/cats/data/ChainCompat.scala | 17 +++++++++++++++++ core/src/main/scala/cats/data/Chain.scala | 17 ++--------------- 3 files changed, 34 insertions(+), 15 deletions(-) create mode 100644 core/src/main/scala-2.12/cats/compat/ChainCompat.scala create mode 100644 core/src/main/scala-2.13+/cats/data/ChainCompat.scala diff --git a/core/src/main/scala-2.12/cats/compat/ChainCompat.scala b/core/src/main/scala-2.12/cats/compat/ChainCompat.scala new file mode 100644 index 0000000000..3d6efc06bb --- /dev/null +++ b/core/src/main/scala-2.12/cats/compat/ChainCompat.scala @@ -0,0 +1,15 @@ +package cats.data + +private[data] trait ChainCompat[+A] { _: Chain[A] => + + /** + * The number of elements in this chain, if it can be cheaply computed, -1 otherwise. + * Cheaply usually means: Not requiring a collection traversal. + */ + final def knownSize: Long = + this match { + case Chain.Empty => 0 + case Chain.Singleton(_) => 1 + case _ => -1 + } +} diff --git a/core/src/main/scala-2.13+/cats/data/ChainCompat.scala b/core/src/main/scala-2.13+/cats/data/ChainCompat.scala new file mode 100644 index 0000000000..2e2be596b7 --- /dev/null +++ b/core/src/main/scala-2.13+/cats/data/ChainCompat.scala @@ -0,0 +1,17 @@ +package cats +package data + +private[data] trait ChainCompat[+A] { _: Chain[A] => + + /** + * The number of elements in this chain, if it can be cheaply computed, -1 otherwise. + * Cheaply usually means: Not requiring a collection traversal. + */ + final def knownSize: Long = + this match { + case Chain.Empty => 0 + case Chain.Singleton(_) => 1 + case Chain.Wrap(seq) => seq.knownSize.toLong + case _ => -1 + } +} diff --git a/core/src/main/scala/cats/data/Chain.scala b/core/src/main/scala/cats/data/Chain.scala index 66b8695197..4c81e22d44 100644 --- a/core/src/main/scala/cats/data/Chain.scala +++ b/core/src/main/scala/cats/data/Chain.scala @@ -32,7 +32,7 @@ import Chain.{ * O(1) `uncons`, such that walking the sequence via N successive `uncons` * steps takes O(N). */ -sealed abstract class Chain[+A] { +sealed abstract class Chain[+A] extends ChainCompat[A] { /** * Returns the head and tail of this Chain if non empty, none otherwise. Amortized O(1). @@ -568,7 +568,7 @@ sealed abstract class Chain[+A] { final def length: Long = this match { case Empty => 0 - case Wrap(seq) => seq.length + case Wrap(seq) => seq.length.toLong case Singleton(a) => 1 case Append(leftNE, rightNE) => leftNE.length + rightNE.length } @@ -578,19 +578,6 @@ sealed abstract class Chain[+A] { */ final def size: Long = length - /** - * The number of elements in this chain, if it can be cheaply computed, -1 otherwise. - * Cheaply usually means: Not requiring a collection traversal. - */ - final def knownSize: Long = - // TODO: consider optimizing for `Chain.Wrap` case – call the underlying `knownSize` method. - // Note that `knownSize` was introduced since Scala 2.13 only. - this match { - case _ if isEmpty => 0 - case Chain.Singleton(_) => 1 - case _ => -1 - } - /** * Compares the length of this chain to a test value. * From 237c2ed5d5e7a3171ad0bfa9c50d2d71026d64bd Mon Sep 17 00:00:00 2001 From: Ben Plommer Date: Thu, 7 Apr 2022 20:47:04 +0100 Subject: [PATCH 3/7] Remove stack-unsafe recursion --- core/src/main/scala/cats/data/Chain.scala | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/src/main/scala/cats/data/Chain.scala b/core/src/main/scala/cats/data/Chain.scala index 4c81e22d44..9a73b18c0e 100644 --- a/core/src/main/scala/cats/data/Chain.scala +++ b/core/src/main/scala/cats/data/Chain.scala @@ -567,10 +567,12 @@ sealed abstract class Chain[+A] extends ChainCompat[A] { */ final def length: Long = this match { - case Empty => 0 - case Wrap(seq) => seq.length.toLong - case Singleton(a) => 1 - case Append(leftNE, rightNE) => leftNE.length + rightNE.length + case Empty => 0 + case Singleton(_) => 1 + case Wrap(seq) => seq.length.toLong + + // TODO: consider implementing this case as a stack-safe recursion. + case Append(_, _) => iterator.length.toLong } /* From d5000f3d643d277ad187eb0e7d54d439e0e48d71 Mon Sep 17 00:00:00 2001 From: Ben Plommer Date: Thu, 7 Apr 2022 20:51:31 +0100 Subject: [PATCH 4/7] Fix scala 3 compilation --- core/src/main/scala-2.13+/cats/data/ChainCompat.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala-2.13+/cats/data/ChainCompat.scala b/core/src/main/scala-2.13+/cats/data/ChainCompat.scala index 2e2be596b7..a02439c188 100644 --- a/core/src/main/scala-2.13+/cats/data/ChainCompat.scala +++ b/core/src/main/scala-2.13+/cats/data/ChainCompat.scala @@ -1,7 +1,7 @@ package cats package data -private[data] trait ChainCompat[+A] { _: Chain[A] => +private[data] trait ChainCompat[+A] { self: Chain[A] => /** * The number of elements in this chain, if it can be cheaply computed, -1 otherwise. From bb245010c9dbe9558a55294f4cdd81ec0c445e4c Mon Sep 17 00:00:00 2001 From: Ben Plommer Date: Thu, 7 Apr 2022 20:55:03 +0100 Subject: [PATCH 5/7] Tail-recursive length loop --- core/src/main/scala/cats/data/Chain.scala | 31 +++++++++++++++++------ 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/core/src/main/scala/cats/data/Chain.scala b/core/src/main/scala/cats/data/Chain.scala index 9a73b18c0e..accce42c1e 100644 --- a/core/src/main/scala/cats/data/Chain.scala +++ b/core/src/main/scala/cats/data/Chain.scala @@ -565,15 +565,30 @@ sealed abstract class Chain[+A] extends ChainCompat[A] { /** * Returns the number of elements in this structure */ - final def length: Long = - this match { - case Empty => 0 - case Singleton(_) => 1 - case Wrap(seq) => seq.length.toLong + final def length: Long = { + @annotation.tailrec + def loop(chains: List[Chain[A]], acc: Long): Long = + chains match { + case Nil => acc + case h :: tail => + h match { + case Empty => loop(tail, acc) + case Wrap(seq) => loop(tail, acc + seq.length) + case Singleton(a) => loop(tail, acc + 1) + case Append(l, r) => loop(l :: r :: tail, acc) + } + } + loop(this :: Nil, 0L) + } - // TODO: consider implementing this case as a stack-safe recursion. - case Append(_, _) => iterator.length.toLong - } + this match { + case Empty => 0 + case Singleton(_) => 1 + case Wrap(seq) => seq.length.toLong + + // TODO: consider implementing this case as a stack-safe recursion. + case Append(_, _) => iterator.length.toLong + } /* * Alias for length From 718dc8d6730408173d83db0da83e6730c327963f Mon Sep 17 00:00:00 2001 From: Ben Plommer Date: Thu, 7 Apr 2022 20:56:09 +0100 Subject: [PATCH 6/7] Remove dead code --- core/src/main/scala/cats/data/Chain.scala | 9 --------- 1 file changed, 9 deletions(-) diff --git a/core/src/main/scala/cats/data/Chain.scala b/core/src/main/scala/cats/data/Chain.scala index accce42c1e..41625eb794 100644 --- a/core/src/main/scala/cats/data/Chain.scala +++ b/core/src/main/scala/cats/data/Chain.scala @@ -581,15 +581,6 @@ sealed abstract class Chain[+A] extends ChainCompat[A] { loop(this :: Nil, 0L) } - this match { - case Empty => 0 - case Singleton(_) => 1 - case Wrap(seq) => seq.length.toLong - - // TODO: consider implementing this case as a stack-safe recursion. - case Append(_, _) => iterator.length.toLong - } - /* * Alias for length */ From 212521832007212c6a8aff9b309b1b1ed1e7bc3e Mon Sep 17 00:00:00 2001 From: Ben Plommer Date: Thu, 7 Apr 2022 20:56:26 +0100 Subject: [PATCH 7/7] Fix comment --- core/src/main/scala/cats/data/Chain.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/cats/data/Chain.scala b/core/src/main/scala/cats/data/Chain.scala index 41625eb794..b302258509 100644 --- a/core/src/main/scala/cats/data/Chain.scala +++ b/core/src/main/scala/cats/data/Chain.scala @@ -581,7 +581,7 @@ sealed abstract class Chain[+A] extends ChainCompat[A] { loop(this :: Nil, 0L) } - /* + /** * Alias for length */ final def size: Long = length