Fix #4177: Generate optimised applyOrElse implementation for partial function literals #4245

allanrenucci · 2018-04-04T16:33:43Z

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

Separate subject from body with a blank line

When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:

Limit the subject line to 72 characters

Capitalize the subject line

Do not end the subject line with a period

Use the imperative mood in the subject line ("Add" instead of "Added")

Wrap the body at 80 characters

Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

Blaisorblade · 2018-04-04T17:50:46Z

compiler/src/dotty/tools/dotc/core/Definitions.scala

@@ -585,6 +585,10 @@ class Definitions {

  lazy val PartialFunctionType: TypeRef         = ctx.requiredClassRef("scala.PartialFunction")
  def PartialFunctionClass(implicit ctx: Context) = PartialFunctionType.symbol.asClass
+
+    lazy val PartialFunction_applyOrElseR = PartialFunctionClass.requiredMethodRef(nme.applyOrElse)


This has the wrong indent size?

This is the indentation scheme used across the file

Members of classes are indented relative to their parent.

Blaisorblade · 2018-04-04T17:51:28Z

compiler/src/dotty/tools/dotc/transform/ExpandSAMs.scala

    val applyFn = applyDef.symbol.asTerm

-    val MethodTpe(paramNames, paramTypes, _) = applyFn.info
+  val MethodTpe(paramNames, paramTypes, _) = applyFn.info


indent issue here too?

allanrenucci · 2018-04-04T18:23:03Z

compiler/src/dotty/tools/dotc/ast/tpd.scala

@@ -274,7 +274,7 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
    val constr = ctx.newConstructor(cls, Synthetic, Nil, Nil).entered
    def forwarder(fn: TermSymbol, name: TermName) = {
      val fwdMeth = fn.copy(cls, name, Synthetic | Method).entered.asTerm


Adding the Override modifier here when needed should fix the test failures. In this PR, we override PartialFunction::applyOrElse. I was not able to reproduce the compiler errors with a minimised test case, only by compiling Dotty itself

allanrenucci · 2018-04-05T11:25:02Z

compiler/src/dotty/tools/dotc/ast/tpd.scala

-      val fwdMeth = fn.copy(cls, name, Synthetic | Method).entered.asTerm
-      DefDef(fwdMeth, prefss => ref(fn).appliedToArgss(prefss))
+      var flags = Synthetic | Method
+      val isOverride = parents.exists(_.member(name).hasAltWith(_.info == fn.info))


Is this the proper way to check if the newly defined symbol overrides something in the parents?

See SymDenotation#allOverriddenSymbols

allanrenucci · 2018-04-05T11:26:43Z

compiler/src/dotty/tools/dotc/transform/ExpandSAMs.scala

-    cpy.Block(tree)(List(applyDef, isDefinedAtDef), anonCls)
+    val applyOrElseDef = transformFollowingDeep(DefDef(applyOrElseFn, applyOrElseRhs(_)))
+
+    val anonCls = AnonClass(tpt.tpe :: Nil, List(applyFn, isDefinedAtFn, applyOrElseFn), List(nme.apply, nme.isDefinedAt, nme.applyOrElse))


I followed the previous scheme. But, is there a reason to use forwarder and not inline apply, isDefinedAtFn and applyOrElseFn in the anonymous class?

I don't think there's a reason. We could implement them directly.

allanrenucci · 2018-04-11T10:06:39Z

test performance please

dottybot · 2018-04-11T10:07:36Z

performance test scheduled: 1 job(s) in queue, 1 running.

dottybot · 2018-04-11T14:30:00Z

performance test failed:

Please check http://lamppc37.epfl.ch:8000/pull-4245-04-11-16.21.out for more information

liufengyun · 2018-04-11T14:58:16Z

@allanrenucci can you try rebase? It seems this PR has some interaction with isInstanceofCheck in the case of partial functions.

allanrenucci · 2018-04-11T17:23:52Z

@liufengyun I was able to minimise the issue. The following code snippet reports an unchecked warning:

class Test {
  def test[X <: Option[Int]](x: X) = x.isInstanceOf[Some[Int]]
}

liufengyun · 2018-04-11T18:09:40Z

Thanks a lot @allanrenucci , I'm investigating it, type inference somehow failed to infer a <: Int, given Some[a] <: X and X <: Option[Int].

allanrenucci · 2018-04-11T19:30:34Z

Blocked by #4297

odersky

scalac only emits two methods for partial function: isDefined and applyOrElse, or if the match is total, apply. This works because it inherits from AbstractPartialFunction which implements apply in terms of applyOrElse and applyOrElse in terms of apply. We should do the same thing. It's potentially expensive to duplicate a large pattern match.

odersky · 2018-04-16T16:51:10Z

compiler/src/dotty/tools/dotc/transform/ExpandSAMs.scala

-    cpy.Block(tree)(List(applyDef, isDefinedAtDef), anonCls)
+    val applyOrElseDef = transformFollowingDeep(DefDef(applyOrElseFn, applyOrElseRhs(_)))
+
+    val anonCls = AnonClass(tpt.tpe :: Nil, List(applyFn, isDefinedAtFn, applyOrElseFn), List(nme.apply, nme.isDefinedAt, nme.applyOrElse))


I don't think there's a reason. We could implement them directly.

allanrenucci · 2018-04-17T10:21:10Z

or if the match is total, apply

@odersky I am not sure to understand what you mean here.

If I compile val x: PartialFunction[Int, Int] = { case x => x } with scalac I get something like:

val x: PartialFunction[Int,Int] = ({
  class $anonfun extends AbstractPartialFunction[Int,Int] {
    override def applyOrElse[A1 <: Int, B1 >: Int](x1: A1, default: A1 => B1): B1 = (x1: @unchecked) match {
      case (x @ _) => x
      case (defaultCase$ @ _) => default.apply(x1)
    }
    def isDefinedAt(x1: Int): Boolean = (x1: @unchecked) match {
      case (x @ _) => true
      case (defaultCase$ @ _) => false
    }
  }
  new $anonfun()
}: PartialFunction[Int,Int])

val x: PartialFunction[Int, Int] = (x: Int) => x doesn't even compile with scalac.

Did you mean that in the second case we can do better than scalac. We know that the match is total an can generate something like:

val x: PartialFunction[Int,Int] = ({
  class $anonfun extends PartialFunction[Int,Int] {
    def apply(x: Int) = x
    def isDefinedAt(x1: Int): Boolean = true
    }
  }
  new $anonfun()
}: PartialFunction[Int,Int])

To know if the match is total, we can use a simple heuristic: if the RHS of the anonymous function is a Match then it is not total otherwise it is

Blaisorblade · 2018-04-17T17:38:03Z

if the RHS of the anonymous function is a Match then it is not total otherwise it is

I assume you include x => { x match { case 1 => x.toString } } as a PartialFunction, per #4241.

(Edited description so the PR is linked in #4177, "Fix #NNNN" in the title doesn't work).

…tial function literals

…rals This is done by extending `scala.runtime.AbstractPartialFunction`.

- `x => x match { case x => x }` is a PF - `x => { x match { case x => x } }` is a PF - `x => { println("foo"); x match { case x => x } }` is not a PF - `x => x` is not a PF

E.g. ``` class Foo(val field: Option[Int]) val p: PartialFunction[Foo, Int] = foo => foo.field match { case Some(x) => x } ```

odersky

I appreciated the crystal-clear code! LGTM.

dottybot reviewed Apr 4, 2018

View reviewed changes

Blaisorblade reviewed Apr 4, 2018

View reviewed changes

allanrenucci commented Apr 4, 2018

View reviewed changes

allanrenucci force-pushed the fix-4177 branch 2 times, most recently from 180cd13 to 641f84e Compare April 5, 2018 10:10

allanrenucci changed the title ~~[WIP] Fix #4177: Generate optimised applyOrElse implementation for partial function literals~~ Fix #4177: Generate optimised applyOrElse implementation for partial function literals Apr 5, 2018

allanrenucci commented Apr 5, 2018

View reviewed changes

allanrenucci requested a review from odersky April 5, 2018 11:28

allanrenucci added the stat:needs review label Apr 5, 2018

scala deleted a comment from dottybot Apr 11, 2018

allanrenucci force-pushed the fix-4177 branch from 641f84e to 59cfae9 Compare April 11, 2018 16:24

allanrenucci removed the request for review from odersky April 11, 2018 17:00

allanrenucci self-assigned this Apr 11, 2018

allanrenucci added stat:wip and removed stat:needs review labels Apr 11, 2018

liufengyun mentioned this pull request Apr 11, 2018

Spurious is instanceOf warning #4297

Closed

allanrenucci force-pushed the fix-4177 branch from 59cfae9 to a9bc3d4 Compare April 11, 2018 19:29

allanrenucci added stat:on hold and removed stat:wip labels Apr 11, 2018

allanrenucci added stat:needs review and removed stat:on hold labels Apr 12, 2018

allanrenucci assigned odersky and unassigned allanrenucci Apr 12, 2018

allanrenucci requested a review from odersky April 12, 2018 12:47

allanrenucci force-pushed the fix-4177 branch from 3d9aec0 to 6644b3f Compare April 12, 2018 16:08

odersky requested changes Apr 16, 2018

View reviewed changes

odersky assigned allanrenucci and unassigned odersky Apr 16, 2018

allanrenucci added stat:wip and removed stat:needs review labels Apr 17, 2018

allanrenucci force-pushed the fix-4177 branch 3 times, most recently from 8cb059e to 4da33ae Compare April 20, 2018 17:25

Fix scala#4177: Generate optimised applyOrElse implementation for par…

ac489db

…tial function literals

allanrenucci force-pushed the fix-4177 branch from 0de4e10 to 21e6aa5 Compare April 25, 2018 13:08

allanrenucci added 3 commits April 25, 2018 15:21

Only generate isDefined and applyOrElse for partial function lite…

b9a7b2f

…rals This is done by extending `scala.runtime.AbstractPartialFunction`.

Polishing

c8904c5

Fix scala#4241: Follow Scalac for partial function literals

d03b226

- `x => x match { case x => x }` is a PF - `x => { x match { case x => x } }` is a PF - `x => { println("foo"); x match { case x => x } }` is not a PF - `x => x` is not a PF

allanrenucci force-pushed the fix-4177 branch from 21e6aa5 to d03b226 Compare April 25, 2018 16:53

Fix partial functions with non trivial selector

8a91774

E.g. ``` class Foo(val field: Option[Int]) val p: PartialFunction[Foo, Int] = foo => foo.field match { case Some(x) => x } ```

allanrenucci added stat:needs review and removed stat:wip labels Apr 26, 2018

allanrenucci assigned odersky and unassigned allanrenucci Apr 26, 2018

odersky approved these changes Apr 27, 2018

View reviewed changes

odersky merged commit 770320b into scala:master Apr 27, 2018

allanrenucci deleted the fix-4177 branch April 27, 2018 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4177: Generate optimised applyOrElse implementation for partial function literals #4245

Fix #4177: Generate optimised applyOrElse implementation for partial function literals #4245

allanrenucci commented Apr 4, 2018 •

edited by Blaisorblade

Loading

dottybot left a comment

Blaisorblade Apr 4, 2018

allanrenucci Apr 5, 2018

odersky Apr 16, 2018

Blaisorblade Apr 4, 2018

allanrenucci Apr 4, 2018

allanrenucci Apr 5, 2018

smarter Apr 11, 2018

allanrenucci Apr 5, 2018

odersky Apr 16, 2018

allanrenucci commented Apr 11, 2018

dottybot commented Apr 11, 2018

dottybot commented Apr 11, 2018

liufengyun commented Apr 11, 2018

allanrenucci commented Apr 11, 2018

liufengyun commented Apr 11, 2018 •

edited

Loading

allanrenucci commented Apr 11, 2018

odersky left a comment

odersky Apr 16, 2018

allanrenucci commented Apr 17, 2018 •

edited

Loading

Blaisorblade commented Apr 17, 2018

odersky left a comment

Fix #4177: Generate optimised applyOrElse implementation for partial function literals #4245

Fix #4177: Generate optimised applyOrElse implementation for partial function literals #4245

Conversation

allanrenucci commented Apr 4, 2018 • edited by Blaisorblade Loading

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allanrenucci commented Apr 11, 2018

dottybot commented Apr 11, 2018

dottybot commented Apr 11, 2018

liufengyun commented Apr 11, 2018

allanrenucci commented Apr 11, 2018

liufengyun commented Apr 11, 2018 • edited Loading

allanrenucci commented Apr 11, 2018

odersky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allanrenucci commented Apr 17, 2018 • edited Loading

Blaisorblade commented Apr 17, 2018

odersky left a comment

Choose a reason for hiding this comment

allanrenucci commented Apr 4, 2018 •

edited by Blaisorblade

Loading

liufengyun commented Apr 11, 2018 •

edited

Loading

allanrenucci commented Apr 17, 2018 •

edited

Loading