Remove PreName add (almost) all other implicit conversions #4077

OlivierBlanvillain · 2018-03-06T13:09:12Z

This is followup to #3986 than removes a few virtual calls that the JIT complained about.

OlivierBlanvillain · 2018-03-06T21:32:55Z

test performance please

dottybot · 2018-03-06T21:33:18Z

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

dottybot · 2018-03-06T23:16:21Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (4be2e76)

odersky · 2018-03-07T16:07:25Z

It does not seem to get us any performance gain. And there is quite a bit of duplication, in particular by duplicating InheritedCache. So I am dubious this is actually an improvement.

regarding PreName: yes we need relatively few overloads if we drop it, but that is not the end of the story. The problem is that in the future there will be constant hassles that a method exists for Name but not for String or vice versa, which will require either annoying conversions or more ad-hoc overloads. So I would actually be in favor of keeping PreName in as well.

OlivierBlanvillain · 2018-03-08T10:35:46Z

@odersky I remove the InheritedCache commit, I agree it was too much duplication.

Regarding the PreName commit I updated it to minimize of number of String method (instead of minimizing the diff size, which I did previously). The result has only 4 overloaded methods that have a overloaded String version, which corresponds to the once that are heavily used in Definitions.scala. For everything else, Strings should be explicitly converted at call site with .toTermName/.toTypeName.

I believe the PreName change is worth doing for readability alone. The change was actually motivated by a real life confusion: @gsps came to my office a few weeks asking "how do I convert a String to PreName?", we spent ~10 minutes pocking around the code trying to figure things out, and finally had to resort to compiling Definitions.scala with -Xprint:typer to understand what was actually going on there.

odersky · 2018-03-09T13:51:20Z

Looking at the diffs I am still not convinced about PreName. I think the code reads worse now than before. Regarding discoverability, let's just put the String instance for PreName next to PreName, then it is never an issue.

PreName was abusing the implementation of implicit class by using the underlying implicit conversion. For instance, several method form Definitions where expecting an argument of type PreName and called with a String. This commit completely removes the PreName abstraction by explicitly calling .toTermName and .toTypeName as required. This change should be beneficial for both readability (more precise types) and performance. Indeed by "pushing out" all the calls to .toTermName and .toTypeName to the call site, these methods will never use virtual dispatch. To keep the concisness of string literals a couple of methods where duplicated to take String arguments (and forward to the TypeName/TermName version).

OlivierBlanvillain · 2018-03-09T18:58:12Z

The few additional .toTypeName and .toTermName are the price to pay to get ride of implicit conversions. If these changes are too much I guess we should abandon the idea of further deprecating them, as it would imply adding import scala.language.implicitConversions on the ~100 files touched by this PR.

I newly pushed commits remove all the other implicit conversions, with the exception of these two to use Symbol as if it was a SymDenotation.

FlagConjunction relyvily relied on implicit conversion and was boxing/unboxing on every use. This commit remove the FlagConjunction abstraction, replacing by the following two constructs: - .isBoth(flagA, and = flagB) instead of is(allOf(flagA, flagB)) - .isBoth(Private, and = Local) instead of is(PrivateLocal) & co - .isClassTypeParam / .isJavaTrait / .isParamForwarder as their translation into flags is non trivial.

nicolasstucki · 2018-03-19T17:01:07Z

.gitignore

@@ -73,3 +73,4 @@ compiler/after-pickling.txt
 *.dotty-ide-version

 *.decompiled.out
+bench/compile.txt


This commit LGTM

nicolasstucki · 2018-03-19T17:02:31Z

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

@@ -134,7 +134,7 @@ object Types {
    }

    /** Is this type different from NoType? */
-    def exists: Boolean = true
+    final def exists: Boolean = !this.eq(NoType)


Use this.ne(NoType), otherwise this commit LGTM

LGTM to me also

nicolasstucki · 2018-03-19T17:04:22Z

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

@@ -91,7 +91,7 @@ object Trees {
    /** The type of the tree. In case of an untyped tree,
     *   an UnAssignedTypeException is thrown. (Overridden by empty trees)
     */
-    def tpe: T @uncheckedVariance = {
+    final def tpe: T @uncheckedVariance = {


This commit LGTM

nicolasstucki · 2018-03-19T17:07:15Z

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

+    def ensureMethodic(implicit ctx: Context): Type = self match {
+      case self: MethodicType => self
+      case _ => if (ctx.erasedTypes) MethodType(Nil, self) else ExprType(self)
+    }
  }
 }


This commit LGTM

nicolasstucki · 2018-03-19T17:09:18Z

compiler/src/dotty/tools/dotc/util/Positions.scala

-  implicit def positionCoord(pos: Position): Coord =
+  def indexCoord(n: Int): Coord = new Coord(n + 1)
+
+  def pos2coord(pos: Position): Coord =


This commit LGTM

Overall I am skeptical about merging this.

nicolasstucki · 2018-03-19T17:10:57Z

compiler/src/dotty/tools/repl/terminal/filters/UndoFilter.scala

+  val undoMsg       = Ansi.Color.Blue(Ansi.Str.parse(" ...undoing last action, `Alt -` or `Esc -` to redo"))
+  val cannotUndoMsg = Ansi.Color.Blue(Ansi.Str.parse(" ...no more actions to undo"))
+  val redoMsg       = Ansi.Color.Blue(Ansi.Str.parse(" ...redoing last action"))
+  val cannotRedoMsg = Ansi.Color.Blue(Ansi.Str.parse(" ...no more actions to redo"))


This commit LGTM

Not sure about this one.

nicolasstucki · 2018-03-19T17:14:12Z

compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala

@@ -716,7 +716,7 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma
    def isDeprecated: Boolean = false
    def isMutable: Boolean = sym is Flags.Mutable
    def hasAbstractFlag: Boolean =
-      (sym is Flags.Abstract) || (sym is Flags.JavaInterface) || (sym is Flags.Trait)
+      (sym is Flags.Abstract) || (sym is Flags.Trait)


Because is(JavaInterface) implies is(Trait): https://github.com/lampepfl/dotty/blob/84bf2fac1aac3f381f2f3f68cc8a7dde538aca95/compiler/src/dotty/tools/dotc/core/Flags.scala#L614

nicolasstucki · 2018-03-19T17:17:39Z

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

-      (fs >>> TYPESHIFT) == (flags.bits >>> TYPESHIFT)
-    }
+    /** Does this flag set have both of the given flags? */
+    def isBoth(flags: FlagSet, and: FlagSet): Boolean =


This commit LGTM

odersky

I went through all commits except the test commits and added my opinion to each.

odersky · 2018-03-26T14:44:59Z

compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala

@@ -154,9 +154,9 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma
  lazy val Predef_classOf: Symbol = defn.ScalaPredefModule.requiredMethod(nme.classOf)


I prefer we leave PreName, but move the String -> PreName conversion to the companion object of PreName.

That doesn't work as moving it the companion of PreName is not enough to make .toTermName and .toTypeName available on Strings. I guess we could have two implicit conversions, one in PreName's companion and one available by import, but I feel it would just make things worse...

Are you sure you don't like this commit as is? In my opinion, completely removing an abstraction from the code base is a clear simplification. The diff might look large but in reality, most of the line changes are about making type more precise by having the TermName/TypeName instead of PreName. The balance sheet in term of added boilerplate is also reasonable:

toTermName added: 22
toTermName removed: 12

toTypeName added: 12
toTypeName removed: 4

+4 overloaded methods with String & Name arguments

I still think we should leave PreName. I don't like to have to overload methods, and the problem is, if you are systematic about it (and you should be!) you will end up with a lot more overloaded methods. PreName is a single abstraction which fixes this quite nicely. For me it is really analogous to IterableOnce in collections. Before we had some overloaded methods that took Iterables and Iterators respectively. At first there were only a few. Then people complained that, by rights, some other methods should also be dual purpose, and then some others more, and so on. The introduction of IterableOnce stopped the bleeding.

I realize it's a contentious issue, but then the maxime "when in doubt, don't change it" applies.

One other thing: We have to tell everyone working on the compiler that Decorators is a highly recommended (maybe mandatory?) input. I have seen so much awkward code because people did not know about it. Is there a good place where we can put it so people will read it?

odersky · 2018-03-26T14:47:05Z

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

@@ -23,6 +23,7 @@ import config.Config
 import util.common._


This commit LGTM

odersky · 2018-03-26T17:01:58Z

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

@@ -134,7 +134,7 @@ object Types {
    }

    /** Is this type different from NoType? */
-    def exists: Boolean = true
+    final def exists: Boolean = !this.eq(NoType)


LGTM to me also

odersky · 2018-03-26T17:04:48Z

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

@@ -685,9 +685,6 @@ object Contexts {



This commit LGTM

odersky · 2018-03-26T17:08:34Z

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

@@ -260,9 +260,9 @@ object desugar {
  @sharable private val synthetic = Modifiers(Synthetic)



I think this commit should not be merged. There is a reason for rawMods/modsDeco: mods should not be called on a typed tree because there mods is typically outdated - one should consult symbol.flags instead. The setup here makes sure that in code working with typed trees using a

import tpd._

import, mods is not visible, because modsDeco is not in scope. So this is a safety measure, not a convenience implicit.

odersky · 2018-03-28T15:41:04Z

compiler/src/dotty/tools/dotc/util/Positions.scala

-  implicit def positionCoord(pos: Position): Coord =
+  def indexCoord(n: Int): Coord = new Coord(n + 1)
+
+  def pos2coord(pos: Position): Coord =


Overall I am skeptical about merging this.

odersky · 2018-03-28T15:41:54Z

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

@@ -601,7 +601,7 @@ class OrderingConstraint(private val boundsMap: ParamBounds,
        " := " ~ tp.toText(printer)
    }


Not an improvement, lots of nitty gritty boilerplate to add.

odersky · 2018-03-28T15:45:33Z

compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala

@@ -828,8 +828,8 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma
    def freshLocal(cunit: CompilationUnit, name: String, tpe: Type, pos: Position, flags: Flags): Symbol =


SymUtils is not used much, to this commit looks smallish. But we should use it more often because it gives us a way to add methods without overloading SymDenotation. In that case I think it is essential that it works for Symbol and SymDenotation in the same way. So my vote is to leave this as is.

odersky · 2018-03-28T15:47:39Z

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

 /** A decorator that provides methods for modeling type application */
-class TypeApplications(val self: Type) extends AnyVal {
+trait TypeApplications extends Any {
+  def self: Type


That will cause a box for every method call. So don't think this is an improvement.

odersky · 2018-03-28T15:48:19Z

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

@@ -47,17 +47,17 @@ object StdNames {
  }

  abstract class DefinedNames[N <: Name] {


I prefer the current version.

allanrenucci · 2018-03-28T16:08:30Z

compiler/test/dotty/tools/repl/ReplTest.scala

@@ -44,12 +44,14 @@ class ReplTest extends ReplDriver(
  def storedOutput(): String =
    stripColor(out.asInstanceOf[StoringPrintStream].flushStored())

-  protected implicit def toParsed(expr: String)(implicit state: State): Parsed = {


I think a better solution is to expose a compile method in ReplTest that takes a String

OlivierBlanvillain force-pushed the cleanup-megamorphic-calls-2 branch 2 times, most recently from d78fefb to e6b24fb Compare March 8, 2018 10:24

OlivierBlanvillain force-pushed the cleanup-megamorphic-calls-2 branch from e6b24fb to e429912 Compare March 9, 2018 18:44

OlivierBlanvillain added 6 commits March 9, 2018 19:44

Add bench/compile.txt to gitignore

1e471b7

Devirtualize Denotation.info

57df5b5

Devirtualize Type.exists

163509d

Devirtualize Tree.tpe

f4e9ccd

Remove a few implicit conversions

a50b2d4

OlivierBlanvillain force-pushed the cleanup-megamorphic-calls-2 branch from e429912 to a33c99d Compare March 9, 2018 18:44

OlivierBlanvillain changed the title ~~Remove PreName~~ Remove PreName add (almost) all other implicit conversions Mar 9, 2018

OlivierBlanvillain added 12 commits March 9, 2018 21:15

Remove modsDeco

b18fba5

Explicit Context to Printer conversion

e07a68d

Explicit SourcePosition to Position conversions

ebe084f

Explicit ModifiersParsingTest.parse

c064915

Explicit Str.parse

9866c74

Explicit Position to Coord conversion

27ee9a4

Explicit Text to String conversion

4f119bc

Remove implicit conversion in repl tests

f2fba0b

Refactor SymUtils.decorate* as an implicit class

c01612a

Refactor decorateTypeApplication as implicit class

0bed6fd

Explicit String to Name conversion in StdNames

01cc814

Remove Context to ContextBase implicit conversion

c49d246

OlivierBlanvillain force-pushed the cleanup-megamorphic-calls-2 branch from a33c99d to c49d246 Compare March 9, 2018 20:22

nicolasstucki reviewed Mar 19, 2018

View reviewed changes

allanrenucci assigned OlivierBlanvillain and odersky Mar 26, 2018

odersky requested changes Mar 28, 2018

View reviewed changes

odersky removed their assignment Mar 28, 2018

allanrenucci reviewed Mar 28, 2018

View reviewed changes

OlivierBlanvillain mentioned this pull request Apr 13, 2018

Remove some implicit conversions #4312

Merged

OlivierBlanvillain closed this Apr 13, 2018

		@@ -154,9 +154,9 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma
		lazy val Predef_classOf: Symbol = defn.ScalaPredefModule.requiredMethod(nme.classOf)

		@@ -260,9 +260,9 @@ object desugar {
		@sharable private val synthetic = Modifiers(Synthetic)

		@@ -601,7 +601,7 @@ class OrderingConstraint(private val boundsMap: ParamBounds,
		" := " ~ tp.toText(printer)
		}

		@@ -828,8 +828,8 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma
		def freshLocal(cunit: CompilationUnit, name: String, tpe: Type, pos: Position, flags: Flags): Symbol =

		@@ -47,17 +47,17 @@ object StdNames {
		}

		abstract class DefinedNames[N <: Name] {

Remove PreName add (almost) all other implicit conversions #4077

Remove PreName add (almost) all other implicit conversions #4077

Uh oh!

Conversation

OlivierBlanvillain commented Mar 6, 2018

Uh oh!

OlivierBlanvillain commented Mar 6, 2018

Uh oh!

dottybot commented Mar 6, 2018

Uh oh!

dottybot commented Mar 6, 2018

Uh oh!

odersky commented Mar 7, 2018

Uh oh!

OlivierBlanvillain commented Mar 8, 2018

Uh oh!

odersky commented Mar 9, 2018

Uh oh!

OlivierBlanvillain commented Mar 9, 2018

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlivierBlanvillain Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

OlivierBlanvillain Apr 12, 2018 •

edited

Loading