Remove patter matches on ImplicitMethodType and JavaMethodType #3212

nicolasstucki · 2017-09-29T16:03:37Z

No description provided.

nicolasstucki · 2017-09-29T16:35:37Z

test performance please

dottybot · 2017-09-29T16:36:07Z

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

dottybot · 2017-09-29T18:12:11Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3212/ to see the changes.

Benchmarks is based on merge(s) with master

allanrenucci · 2017-09-29T19:47:56Z

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

@@ -288,6 +288,18 @@ object Types {
    /** Is this an alias TypeBounds? */
    final def isAlias: Boolean = this.isInstanceOf[TypeAlias]

+    /** Is this an MethodType which has implicit parameters */


allanrenucci · 2017-09-29T19:48:14Z

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

+      case _ => false
+    }
+
+    /** Is this an MethodType which has implicit parameters */


nicolasstucki · 2017-09-30T12:18:44Z

test performance please

dottybot · 2017-09-30T12:19:01Z

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

dottybot · 2017-09-30T13:20:13Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3212/ to see the changes.

Benchmarks is based on merge(s) with master

nicolasstucki · 2017-09-30T15:09:10Z

test performance please

dottybot · 2017-09-30T15:09:42Z

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

dottybot · 2017-09-30T16:10:37Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3212/ to see the changes.

Benchmarks is based on merge(s) with master

nicolasstucki · 2017-09-30T17:20:52Z

It looks like this change makes the compilation of the dotty compiler 8% faster.

smarter · 2017-09-30T17:24:26Z

Probably not :). Points are missing for today's PRs, same as #3216 (comment)

smarter · 2017-09-30T17:30:17Z

@liufengyun Many recent PRs are missing from https://liufengyun.github.io/bench/, any idea why?

odersky · 2017-10-01T17:47:23Z

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

-    override def isImplicit = true
-    override protected def prefixString = "ImplicitMethodType"
-  }
+  final class ConcreteMethodType(paramNames: List[TermName])(paramInfosExp: MethodType => List[Type], resultTypeExp: MethodType => Type, val kind: MethodKinds.MethodKind)


To keep with naming conventions, it should be CachedMethodType.

odersky · 2017-10-01T17:48:28Z

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

@@ -2788,6 +2784,11 @@ object Types {
         tl => tl.integrate(params, resultType))
    }

+    final def apply(paramNames: List[TermName])(paramInfosExp: MethodType => List[Type], resultTypeExp: MethodType => Type)(implicit ctx: Context): MethodType = {
+      val mtc = unique(new ConcreteMethodType(paramNames)(paramInfosExp, resultTypeExp, kind))
+      if (kind is JavaKind) mtc else checkValid(mtc)


No need for the condition. You can always do the checkValid.

odersky · 2017-10-01T18:10:38Z

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

-      checkValid(unique(new ImplicitMethodType(paramNames)(paramInfosExp, resultTypeExp)))
+  object MethodType extends MethodTypeCompanion { self =>
+    import MethodKinds._
+


Since MethodTypes are created very often (~500K for Dotty bootstrap) and there are only a small number of different ones, I would not do the withKind scaffolding. I would create a finite number of companions instead and use the kind as a parameter.

Actually, I think a simpler scheme is to forgo kinds altogether and only use companions.
Yes, it will get a little bit more complicated once we add unused, but it's still much easier than defining separate kinds.

That design is a lot simpler. The only drawback is that we will have an additional indirection to know is a type is a Java method type or implicit method type as that knowlage is in the companion.

nicolasstucki · 2017-10-02T06:10:12Z

test performance please

dottybot · 2017-10-02T06:10:37Z

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

dottybot · 2017-10-02T07:12:18Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3212/ to see the changes.

Benchmarks is based on merging with master (1dda6e1)

nicolasstucki · 2017-10-03T05:57:54Z

All requested changes have been made.

nicolasstucki · 2017-10-04T16:03:22Z

Squashed and rebased code

Code has changed based on this feedback

odersky

I don't think the withKind with the two boolean parameters is an improvement. I'd drop that and use the old way of choosing companions.

Otherwise LGTM

odersky · 2017-10-04T19:22:41Z

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

@@ -2845,18 +2843,19 @@ object Types {
  }

  object MethodType extends MethodTypeCompanion {
-    def apply(paramNames: List[TermName])(paramInfosExp: MethodType => List[Type], resultTypeExp: MethodType => Type)(implicit ctx: Context): MethodType =
-      checkValid(unique(new CachedMethodType(paramNames)(paramInfosExp, resultTypeExp)))
+    def withKind(isJava: Boolean = false, isImplicit: Boolean = false): MethodTypeCompanion = {


I don't think you need that method anymore

odersky · 2017-10-04T19:23:30Z

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

+  abstract class MethodTypeCompanion extends TermLambdaCompanion[MethodType] { self =>
+
+    def isJava: Boolean = false
+    def isImplicit: Boolean = false


I don't think these are needed either?

They will become useful if we add more method types. I will remove it and add it back if it is needed in the future.

odersky · 2017-10-04T19:24:07Z

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

  }

  object JavaMethodType extends MethodTypeCompanion {
-    def apply(paramNames: List[TermName])(paramInfosExp: MethodType => List[Type], resultTypeExp: MethodType => Type)(implicit ctx: Context): MethodType =
-      unique(new JavaMethodType(paramNames)(paramInfosExp, resultTypeExp))
+    override def isJava: Boolean = true


Can be dropped

odersky · 2017-10-04T19:24:14Z

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

  }

  object ImplicitMethodType extends MethodTypeCompanion {
-    def apply(paramNames: List[TermName])(paramInfosExp: MethodType => List[Type], resultTypeExp: MethodType => Type)(implicit ctx: Context): MethodType =
-      checkValid(unique(new ImplicitMethodType(paramNames)(paramInfosExp, resultTypeExp)))
+    override def isImplicit: Boolean = true


Can be dropped

odersky · 2017-10-04T19:25:00Z

compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala

@@ -774,8 +774,7 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas
        def isImplicit =
          tag == IMPLICITMETHODtpe ||
          params.nonEmpty && (params.head is Implicit)
-        val maker = if (isImplicit) ImplicitMethodType else MethodType


I'd leave as is.

odersky · 2017-10-05T08:53:30Z

Now we got it to be smaller than before Great! LGTM.

nicolasstucki · 2017-10-05T15:05:09Z

Rebased

nicolasstucki added the stat:wip label Sep 29, 2017

nicolasstucki added stat:needs review and removed stat:wip labels Sep 29, 2017

nicolasstucki requested a review from odersky September 29, 2017 17:40

allanrenucci reviewed Sep 29, 2017

View reviewed changes

nicolasstucki force-pushed the simplify-method-types branch from 5190981 to ff309b9 Compare September 30, 2017 12:18

nicolasstucki force-pushed the simplify-method-types branch from ff309b9 to 3c63040 Compare September 30, 2017 12:54

nicolasstucki assigned odersky Sep 30, 2017

odersky previously requested changes Oct 1, 2017

View reviewed changes

nicolasstucki force-pushed the simplify-method-types branch from 0465c14 to ff6e675 Compare October 4, 2017 16:03

odersky reviewed Oct 4, 2017

View reviewed changes

nicolasstucki added 2 commits October 5, 2017 17:03

Remove ImplicitMethodType and JavaMethodType

81dc91d

Remove unneeded abstractions

4811889

nicolasstucki force-pushed the simplify-method-types branch from 1efead3 to 4811889 Compare October 5, 2017 15:04

nicolasstucki force-pushed the simplify-method-types branch from e07da93 to 4811889 Compare October 5, 2017 15:20

Fix implicit MethodType guard

d39513e

odersky assigned nicolasstucki and unassigned odersky Oct 7, 2017

nicolasstucki merged commit 70eb2b9 into scala:master Oct 8, 2017

allanrenucci deleted the simplify-method-types branch December 14, 2017 19:24

Remove patter matches on ImplicitMethodType and JavaMethodType #3212

Remove patter matches on ImplicitMethodType and JavaMethodType #3212

Uh oh!

Conversation

nicolasstucki commented Sep 29, 2017

Uh oh!

nicolasstucki commented Sep 29, 2017

Uh oh!

dottybot commented Sep 29, 2017

Uh oh!

dottybot commented Sep 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasstucki commented Sep 30, 2017

Uh oh!

dottybot commented Sep 30, 2017

Uh oh!

dottybot commented Sep 30, 2017

Uh oh!

nicolasstucki commented Sep 30, 2017

Uh oh!

dottybot commented Sep 30, 2017

Uh oh!

dottybot commented Sep 30, 2017

Uh oh!

nicolasstucki commented Sep 30, 2017

Uh oh!

smarter commented Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented Sep 30, 2017

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!

nicolasstucki commented Oct 2, 2017

Uh oh!

dottybot commented Oct 2, 2017

Uh oh!

dottybot commented Oct 2, 2017

Uh oh!

nicolasstucki commented Oct 3, 2017

Uh oh!

nicolasstucki commented Oct 4, 2017

Uh oh!

odersky 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!

odersky commented Oct 5, 2017

Uh oh!

nicolasstucki commented Oct 5, 2017

Uh oh!

Uh oh!

smarter commented Sep 30, 2017 •

edited

Loading