Make Predicate stack-safe using Eval #283
Conversation
bbjubjub2494
changed the title
There was a problem hiding this comment.
I'm concerned with the drop in performance here. Can you make a benchmark?
There are other ways to make Predicate stack safe without giving up all the performance (see what was done in cats): https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/data/AndThen.scala
Will get back to you on that. I think a mix between using AndThen where possible and Eval otherwise might work. |
Codecov Report
@@ Coverage Diff @@
## master #283 +/- ##
==========================================
- Coverage 91.5% 91.42% -0.09%
==========================================
Files 24 24
Lines 1625 1632 +7
Branches 214 219 +5
==========================================
+ Hits 1487 1492 +5
- Misses 138 140 +2
Continue to review full report at Codecov.
|
bbjubjub2494
changed the title
bbjubjub2494
changed the title
|
The PR stabilized and finalized. @johnynek would you mind reviewing again or pinging somebody else? |
|
|
||
| /** | ||
| * Return the opposite predicate | ||
| */ | ||
| def unary_!(): Predicate[A] = negate | ||
|
|
||
| /** | ||
| * compose with a function the predicate. |
There was a problem hiding this comment.
| * compose with a function the predicate. | |
| * compose a function with the predicate. |
typo on my part, will fix promptly
| */ | ||
| abstract class Predicate[-A] extends scala.Function1[A, Boolean] { self => | ||
| final class Predicate[-A] private(private val p: AndThen[A, Eval[Boolean]]) { self => |
There was a problem hiding this comment.
I'm still pretty skeptical of the need for both AndThen and Eval. Eval comes with a big performance penalty, like 100x isn't uncommon.
Is blowing the stack very common here?
Next, Why not just make type Predicate[-A] = Kleisli[Eval, A, Boolean] and then have some methods on that if you want to have a stack safe predicate? Why a new class?
There was a problem hiding this comment.
Regarding performance, I'm not seeing any 100x: I posted a benchmark in the opening post and it seems to be at 2x worst case scenario. I might be missing something though.
Regarding the need for Eval, since we are dealing with tree-shaped expressions, there is a need for some type of trampoline like Eval if stack safety is to be achieved. (A Defer[_] to be precise) Maybe scala.util.control.TailCalls.TailRec would be preferable since we dont need all the laziness modes?
Regarding AndThen, it makes contramap stack-safe, and it's a thin layer over functions that doesn't even kick in unless a lot of composition is done, so I think it's actually more performant in this role than Kleisli because Kleisli leans on the trampoline. I'll come with benchmarks though.
Regarding whether it's common to blow the stack here, I come at it from the perspective that it shouldn't be possible at all in a Cats library. It should be as efficient as possible while remaining SOE-less. That's my interpretation of the "Efficency" motivation on the Cats page.
Regarding the class question that's a good point. I tried turning the class into an opaque newtype a couple of months ago, but it wouldn't compile under 2.11. Now that 2.11 support has been dropped we could go that route, or do a transparent type alias.
There was a problem hiding this comment.
Regarding trampolines, I realized the most suitable trampoline is probably a dedicated one: we only use Eval.{True, False}, defer, and flatMap, so rolling my own should be fairly easy and would have built-in unboxed booleans. We shall see on the benchmarks.
There was a problem hiding this comment.
Sorry, I overlooked the benchmarks, those look good to me. My concerns are removed by those.
I don't think a custom trampoline can speed this up (maybe I'm wrong) since there are only two Booleans boxing them is almost free, and Eval.True and Eval.False have already boxed them. Eval is faster than scala TailCall and cats.free.Free (by a significant margin) so beating it seems tough to me.
There was a problem hiding this comment.
I'm now seeing a combination of the trampoline and the AST, similar to a trampoline with a monadReader capability. It's probably a tiny bit faster, it's more elegant and allows to apply laws so I'll try that next. Thanks you for stimulating my thought process.
| def negate: Predicate[A] = Predicate.wrap[A](p.andThen { | ||
| case e: Now[Boolean] => if (e.value) Eval.False else Eval.True | ||
| case e => e.flatMap { if (_) Eval.False else Eval.True } | ||
| }) |
There was a problem hiding this comment.
if we had case classes you could defer the code here:
case class Negate[A](of: Predicate[A]) extends Predicate[A] {
...
}then def negate could apply the law negate(negate(p)) == p. Similar with keeping And, Or and Const nodes around.
There was a problem hiding this comment.
I'm not really in favor of adding an AST layer.
I feel like most of the time these kinds of optimisations will be spotted by user code that also knows these laws, and that most of the nodes will be opaque predicates (A => Boolean) that we cant really do a lot with besides double negation. So the laws would rarely be applied in practice. This also clashes with the type alias route, so simple predicates get an additional indirection, and adds a bunch of boilerplate and complexity.
Side note: would we apply De Morgan's laws in either direction?
If the set is simplified it defeats the point
|
Ok there we go. Kleisli is much nicer to work with. We have AST simplification, a more complete scalacheck generator, and the Boolean algebra. I haven't run final benchmarks yet but I think performance profile is the same as 2 days ago. |
Codecov Report
@@ Coverage Diff @@
## master #283 +/- ##
==========================================
- Coverage 91.50% 91.49% -0.02%
==========================================
Files 24 24
Lines 1625 1646 +21
Branches 214 207 -7
==========================================
+ Hits 1487 1506 +19
- Misses 138 140 +2
Continue to review full report at Codecov.
|
| * build a set from a membership function. | ||
| */ | ||
| def apply[A](p: A => Boolean): Predicate[A] = Lift { | ||
| Kleisli(a => Eval.now(p(a))) |
There was a problem hiding this comment.
I think a => if (p(a)) Eval.True else Eval.False may be marginally more efficient.
| @@ -76,6 +160,15 @@ trait PredicateInstances { | |||
| override def combine(l: Predicate[A], r: Predicate[A]): Predicate[A] = l union r | |||
| } | |||
|
|
|||
| implicit def predicateBool[A]: Bool[Predicate[A]] = new Bool[Predicate[A]] { | |||
There was a problem hiding this comment.
we usually give longer names in cats to avoid the name aliasing problem with implicits.
e.g. implicit def catsCollectionsPredicateBool: Bool[Predicate[A]] = ...
| @@ -22,6 +24,16 @@ class PredicateSpec extends CatsSuite { | |||
| checkAll("ContravariantMonoidal[Predicate]", ContravariantMonoidalTests[Predicate].contravariantMonoidal[Int, Int, Int]) | |||
| } | |||
|
|
|||
| { | |||
| implicit val eqForPredicateInt: Eq[Predicate[Int]] = new Eq[Predicate[Int]] { | |||
| val sample = -1 to 1 // need at least 2 elements to distinguish in-between values | |||
There was a problem hiding this comment.
why are just two enough? Why not (-100 to 100) or something?
Or, better yet, why not test with Predicate[Byte] and enumerate all 256 possibilities to check equality.
There was a problem hiding this comment.
If the domain is just one element, then every predicate is effectively equivalent to either Empty or Everything depending on whether it accepts the one element.
I don't think we need a 201 or 256 cardinality domain because I tried manually introducing some defects in Predicate and it caught them immediately with the 3 elements.
That being said, it doesn't negatively affect the performance of the testsuite on my machine at all so I'd be ok with either.
Benchmarkstip of PRsame benchmark against on masterWoah that's not good is it? 13x loss, how did that happen? |
benchmark without Kleislistill 5x |
| def apply[A](p: A => Boolean): Predicate[A] = Lift { | ||
| Kleisli(a => if (p(a)) Eval.True else Eval.False) | ||
| } | ||
|
|
There was a problem hiding this comment.
what about a def fromKleisli[A](k: Kleisli[Eval, A, Boolean]): Predicate[A] = Lift(k)
Fixes last comment in #142, Also adds
contramap()to compensate for the loss of inheritance fromFunction1.Benchmarks
pre-existing 0.9.0 stack-unsafe implementation
(at 992770b)
(SOE at n = 10,000)
this PR (mark 3)
That's roughly -50% at n = 100 and -45% at n = 1,000. It was a convoluted intensive example with all the stress on the internals, so I would consider it a worst case scenario. Most real world intensional sets probably put most of their complexity in the basic predicates rather than in thousands of combinations. Hence I would say -50% is the lower bound of the performance degradation in general. Is that acceptable?