Use applyOrElse for PartialFunctions #6799

szeiger · 2018-06-14T13:14:04Z

This avoids evaluating the guards twice. It can be significantly
(25% in my benchmarks) slower in the worst case (a PF with a single
clause with very efficient guard that usually does not match) but can
have much larger performance gains in other scenarios.

All flatMap-based implementations suitable for lazy collection types
are replaced by a new View.Collect primitive that uses an Iterator
instead, which is considerably faster.

Fixes scala/bug#10901

viktorklang · 2018-06-14T13:48:50Z

src/library/scala/collection/View.scala

@@ -211,6 +211,12 @@ object View extends IterableFactory[View] {
    def iterator = underlying.iterator.flatMap(f)
  }

+  /** A view that collects elements of the underlying collection. */
+  @SerialVersionUID(3L)
+  class Collect[+A, B](underlying: SomeIterableOps[A], pf: PartialFunction[A, B]) extends AbstractView[B] {


None of those classes are. More specific View types like IndexedSeqView have to be able to extend them.

allanrenucci

Should we update PartialFunction to use a local marker instead of a self returning function?

scala/src/library/scala/PartialFunction.scala

Lines 195 to 218 in 80ee48b

    
             /** To implement patterns like {{{ if(pf isDefinedAt x) f1(pf(x)) else f2(x) }}} efficiently 
        
              *  the following trick is used: 
        
              * 
        
              *  To avoid double evaluation of pattern matchers & guards `applyOrElse` method is used here 
        
              *  instead of `isDefinedAt`/`apply` pair. 
        
              * 
        
              *  After call to `applyOrElse` we need both the function result it returned and 
        
              *  the fact if the function's argument was contained in its domain. The only degree of freedom we have here 
        
              *  to achieve this goal is tweaking with the continuation argument (`default`) of `applyOrElse` method. 
        
              *  The obvious way is to throw an exception from `default` function and to catch it after 
        
              *  calling `applyOrElse` but I consider this somewhat inefficient. 
        
              * 
        
              *  I know only one way how you can do this task efficiently: `default` function should return unique marker object 
        
              *  which never may be returned by any other (regular/partial) function. This way after calling `applyOrElse` you need 
        
              *  just one reference comparison to distinguish if `pf isDefined x` or not. 
        
              * 
        
              *  This correctly interacts with specialization as return type of `applyOrElse` 
        
              *  (which is parameterized upper bound) can never be specialized. 
        
              * 
        
              *  Here `fallback_pf` is used as both unique marker object and special fallback function that returns it. 
        
              */ 
        
             private[this] val fallback_pf: PartialFunction[Any, Any] = { case _ => fallback_pf } 
        
             private def checkFallback[B] = fallback_pf.asInstanceOf[PartialFunction[Any, B]] 
        
             private def fallbackOccurred[B](x: B) = (fallback_pf eq x.asInstanceOf[AnyRef])

allanrenucci · 2018-06-14T13:34:10Z

src/library/scala/collection/Iterator.scala

@@ -416,7 +417,8 @@ trait Iterator[+A] extends IterableOnce[A] with IterableOnceOps[A, Iterator, Ite

  def collect[B](pf: PartialFunction[A, B]): Iterator[B] = new AbstractIterator[B] {
    // Manually buffer to avoid extra layer of wrapping with buffered
-    private[this] var hd: A = _
+    private[this] var hd: B = _
+    private[this] final val marker = ScalaRunTime.pfMarker // this should be static if there was a way to make it


Does this need to be a field? Why not a local variable in hasNext?

A local variable would have to be initialized on every call to hasNext. This requires reading an object's field which is slower than a regular instance field. Ideally it should be a static field.

I would suggest defining pfMarker in scala.runtime.Statics and removing this field.

viktorklang · 2018-06-14T13:49:43Z

src/library/scala/util/Try.scala

-    try { if (pf isDefinedAt exception) Success(pf(exception)) else this } catch { case NonFatal(e) => Failure(e) }
-  override def recoverWith[U >: T](pf: PartialFunction[Throwable, Try[U]]): Try[U] =
-    try { if (pf isDefinedAt exception) pf(exception) else this } catch { case NonFatal(e) => Failure(e) }
+  override def recover[U >: T](pf: PartialFunction[Throwable, U]): Try[U] = {


Great catch, Stefan!

julienrf · 2018-06-19T19:32:48Z

src/library/scala/collection/immutable/LongMap.scala

@@ -476,7 +476,7 @@ sealed abstract class LongMap[+T] extends AbstractMap[Long, T]
  override def ++ [V1 >: T](that: scala.collection.Iterable[(Long, V1)]): LongMap[V1] = concat(that)

  def collect[V2](pf: PartialFunction[(Long, T), (Long, V2)]): LongMap[V2] =
-    flatMap(kv => if (pf.isDefinedAt(kv)) new View.Single(pf(kv)) else View.Empty)
+    LongMap.from(new View.Collect(coll, pf))


Why not builder-based implementations instead?

Yes, builder-based would probably be better.

I'll use #6776 (which I just merged) for this purpose.

szeiger · 2018-06-28T10:44:58Z

I've added a commit to build speclib locally instead of using a pre-built binary. The old version was no longer compatible.

szeiger · 2018-06-28T13:12:37Z

See scala/bug#7822 and 94166682ee for context for the speclib stuff.

- Build automatically from sources instead of fetching a pre-built binary which can get out of date and needs to be updated manually - Remove outdated build script (from the ant days)

This avoids evaluating the guards twice. It can be significantly (25% in my benchmarks) slower in the worst case (a PF with a single clause with very efficient guard that usually does not match) but can have much larger performance gains in other scenarios. All flatMap-based implementations suitable for lazy collection types are replaced by a new `View.Collect` primitive that uses an Iterator instead, which is considerably faster.

szeiger · 2018-07-20T14:12:27Z

Rebased and updated to use the reusable strict implementations. I've also moved pfMarker into Statics.

Should we update PartialFunction to use a local marker instead of a self returning function?

The common wisdom so far was that a self-returning function is faster. My benchmarks show the opposite but they only test the collection-specific use-cases. I didn't want to make such a change without more data.

scala-jenkins added this to the 2.13.0-M5 milestone Jun 14, 2018

viktorklang reviewed Jun 14, 2018

View reviewed changes

allanrenucci reviewed Jun 14, 2018

View reviewed changes

viktorklang reviewed Jun 14, 2018

View reviewed changes

julienrf reviewed Jun 19, 2018

View reviewed changes

julienrf approved these changes Jun 19, 2018

View reviewed changes

szeiger force-pushed the issue/10901 branch from 1699b17 to 765a15f Compare June 28, 2018 09:39

szeiger mentioned this pull request Jun 28, 2018

Optimizing HashMap.getOrElse*, Map equality, and Seq sameElements #6856

Merged

szeiger added 2 commits July 20, 2018 14:49

Build speclib/instrumented.jar for partest

4070df4

- Build automatically from sources instead of fetching a pre-built binary which can get out of date and needs to be updated manually - Remove outdated build script (from the ant days)

szeiger force-pushed the issue/10901 branch from 765a15f to a7ac901 Compare July 20, 2018 14:08

szeiger merged commit 21afb09 into scala:2.13.x Jul 27, 2018

szeiger deleted the issue/10901 branch July 27, 2018 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use applyOrElse for PartialFunctions #6799

Use applyOrElse for PartialFunctions #6799

Uh oh!

szeiger commented Jun 14, 2018

Uh oh!

viktorklang Jun 14, 2018

Uh oh!

szeiger Jun 28, 2018

Uh oh!

allanrenucci left a comment

Uh oh!

allanrenucci Jun 14, 2018

Uh oh!

szeiger Jun 22, 2018

Uh oh!

retronym Jun 29, 2018

Uh oh!

viktorklang Jun 14, 2018

Uh oh!

julienrf Jun 19, 2018

Uh oh!

szeiger Jun 22, 2018

Uh oh!

szeiger Jul 19, 2018

Uh oh!

szeiger commented Jun 28, 2018

Uh oh!

szeiger commented Jun 28, 2018

Uh oh!

szeiger commented Jul 20, 2018

Uh oh!

Uh oh!

	/** To implement patterns like {{{ if(pf isDefinedAt x) f1(pf(x)) else f2(x) }}} efficiently
	* the following trick is used:
	*
	* To avoid double evaluation of pattern matchers & guards `applyOrElse` method is used here
	* instead of `isDefinedAt`/`apply` pair.
	*
	* After call to `applyOrElse` we need both the function result it returned and
	* the fact if the function's argument was contained in its domain. The only degree of freedom we have here
	* to achieve this goal is tweaking with the continuation argument (`default`) of `applyOrElse` method.
	* The obvious way is to throw an exception from `default` function and to catch it after
	* calling `applyOrElse` but I consider this somewhat inefficient.
	*
	* I know only one way how you can do this task efficiently: `default` function should return unique marker object
	* which never may be returned by any other (regular/partial) function. This way after calling `applyOrElse` you need
	* just one reference comparison to distinguish if `pf isDefined x` or not.
	*
	* This correctly interacts with specialization as return type of `applyOrElse`
	* (which is parameterized upper bound) can never be specialized.
	*
	* Here `fallback_pf` is used as both unique marker object and special fallback function that returns it.
	*/
	private[this] val fallback_pf: PartialFunction[Any, Any] = { case _ => fallback_pf }
	private def checkFallback[B] = fallback_pf.asInstanceOf[PartialFunction[Any, B]]
	private def fallbackOccurred[B](x: B) = (fallback_pf eq x.asInstanceOf[AnyRef])

Use applyOrElse for PartialFunctions #6799

Use applyOrElse for PartialFunctions #6799

Uh oh!

Conversation

szeiger commented Jun 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szeiger commented Jun 28, 2018

Uh oh!

szeiger commented Jun 28, 2018

Uh oh!

szeiger commented Jul 20, 2018

Uh oh!

Uh oh!