Local optimisations round two #2647

OlivierBlanvillain · 2017-06-01T12:29:23Z

Follow up PR of #2513

Objective: bootstrap Dotty with -optimise!

smarter · 2017-06-01T14:02:45Z

compiler/src/dotty/tools/dotc/transform/localopt/BubbleUpNothing.scala

+
+/** Every pure statement preceding a ??? can be removed.
+ *
+ *  This optimisation makes it rather tricky meaningful examples since the


This sentence seems to be missing some words.

smarter · 2017-06-01T14:05:10Z

compiler/src/dotty/tools/dotc/transform/localopt/Optimisation.scala

+  def visitor: Tree => Unit
+
+  /** Does the actual Tree => Tree transformation, possibly using a different
+   *  context from the one using in Optimisation.


Could you give more explanations on what contexts are used when? Also typo: using -> used.

Got addressed in this PR, we always use the correct, most local context!

DarkDimius · 2017-06-02T09:50:57Z

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

-      def localOptimizations(classNode: ClassNode): Unit = {
-        // BackendStats.timed(BackendStats.methodOptTimer)(localOpt.methodOptimizations(classNode))
+      def localOptimisations(classNode: ClassNode): Unit = {
+        // BackendStats.timed(BackendStats.methodOptTimer)(localOpt.methodOptimisations(classNode))


I propose not to rename the method here. This is how method is actually called in the backend.

DarkDimius · 2017-06-02T09:53:39Z

@OlivierBlanvillain, If you don't mind, I propose to wait with getting this PR in.
I've done quite some changes to Simplify to enable more optimizations and add more documentation.
Would you mind if I take your changes and apply them by hand on my branch?(after my branch passes the tests)

DarkDimius · 2017-06-02T09:55:22Z

For curiosity I've just checked: Oxford English Dictionary, prefers the -ize. But I don't really care. I'm fine with the change.
More details for curious ones: https://www.quora.com/Is-there-any-difference-between-optimization-and-optimisation/answer/Bob-Crowl

OlivierBlanvillain · 2017-06-02T10:03:14Z

@DarkDimius How large is your diff? I would be surprise if git manages to rebase either diffs on top of the other, and if it doesn't, it's probably easier to hand apply your changes on top of this rather than the other way around... (Happy to help if you point me to a branch)

I really don't have a preference between -ise and -ize, as long has it's not half/half 😄

OlivierBlanvillain · 2017-06-07T11:35:59Z

@DarkDimius Found the issue with DropNoEffects, if you want to have a look a the last commits. It's currently not tested by the CI, but this is what I use locally:

$ sbt ';project dotty-compiler-bootstrapped ;clean ;set scalacOptions ++= Seq("-optimise", "-Yopt-phases:DropNoEffects") ;test'

DarkDimius · 2017-06-07T11:41:56Z

@OlivierBlanvillain, nice find! Did tests pass on your machine after the change?

OlivierBlanvillain · 2017-06-07T11:44:30Z

The above, i.e. bootstrapped Dotty tests with -optimise -Yopt-phases:DropNoEffects does. That's a tiny step but I'm confident we can make it work will all optimisations enabled!

DarkDimius · 2017-06-07T12:13:08Z

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

@@ -447,6 +448,8 @@ class Definitions {
    def Long_* = Long_mulR.symbol
    lazy val Long_divR   = LongClass.requiredMethodRef(nme.DIV, List(LongType))
    def Long_/ = Long_divR.symbol
+  val CommutativePrimitiveOperations = new PerRun[collection.Set[Symbol]](implicit ctx =>


Don't put stuff inside Definitions, unless its frequently used by multiple phases.

Same applies for SeqFactoryType.

smarter · 2017-06-07T13:38:12Z

compiler/src/dotty/tools/dotc/transform/localopt/InlineCaseIntrinsics.scala

+                     a.symbol.extendedOverriddenSymbols.isEmpty  &&
+                     (isPureExpr(a.fun) || a.fun.symbol.is(Synthetic)) =>
+
+      def reciever(t: Tree): Type = t match {


typo: reciever -> receiver

smarter · 2017-06-07T13:38:49Z

compiler/src/dotty/tools/dotc/transform/localopt/InlineCaseIntrinsics.scala

+      Block(recv :: Nil, res)
+  }
+
+  // To run this optimisation after erasure one would need to specialize it


Move this comment to the top of the class?

smarter · 2017-06-07T13:41:56Z

compiler/src/dotty/tools/dotc/transform/localopt/InlineLabelsCalledOnce.scala

+        case Some(_) =>
+          a
+      }
+    case a: DefDef if (a.symbol.is(Label) && timesUsed.getOrElse(a.symbol, 0) == 1 && defined.contains(a.symbol)) =>


This same if expression is repeated below, factor it out in a def?

DarkDimius · 2017-06-11T11:17:31Z

@OlivierBlanvillain, i've pushed a merge of our changes. I've quite substantially changed the structure of the split, because the original split introduced 70% slowdown(even after removing fuel)
Now, there is only 20% slowdown. I'm still not sure if this split is worth this slowdown. I'd prefer to keep this phase fast, as it has to run multiple times during the pipeline and it should "pay for itself" by speeding up other phases. If this phase becomes 70% slower, it's harder for it to pay for itself.

smarter · 2017-06-11T11:24:38Z

What changes in the split cause a slowdown?

DarkDimius · 2017-06-11T11:28:16Z

50% that I've removed - substantially more allocations. More maps, sets & etc were allocated, unlike before. More lambdas were coined. More calls were non-local and cold. E.g. there were two contexts being used interchangeably.

In general: abstractions in Scala are anything but free. What brings the remaining 20% slowdown - I haven't checked yet and I don't think I'll have opportunity to do it in coming months.

OlivierBlanvillain · 2017-06-11T13:39:34Z

@DarkDimius Interesting, what measure did you use?

OlivierBlanvillain · 2017-06-12T13:02:59Z

@DarkDimius I just force pushed to restore history, I authored 4 commits under you name, you can check that nothing got lost using git diff a386f63 f8cf56f.

I isolated the CI failure, it looks like compileStdLib compiles forever with -optimise -Yopt-phases:Devalify starting from commit 93e1b6d (Collapse ctx and localCtx together). I guess Devalify really need to run using the context from the overall DefDef tree instead of the local one. Note that before the split, the ctx used by every Visitor/Transformer pair is fixed in the in outer loop, here (which corresponds to the class parameter you instantiated), the localCtx was almost never used before that commit, maybe we should try getting ride of this one instead.

@DarkDimius

@DarkDimius I think you have mesured the wrong with this 20% slowdown. By not sharing instances Optimistion instead of allocating new ones you where also sharing the mutable.Hashmap instances. The failure on compileStdLib is actaully a proper timeout: keeping these Hashmaps around when compiling all of stdlib prevented GC from doing it job, and (I'm guessing) spent a lot of time reshufuling the Hashmap. I also removed your `private val useFuel = false` back to a simple runtime setting check. I moved the debuging code in a `printIfDifferent` which only adds 3 extra bytecode instructions to the body of the TreeMap.transform (going from 70 to 73).

DarkDimius · 2017-06-14T08:57:59Z

Let's get this in. The followup discussions can be held separately, but this PR in any case is an improvement.

DarkDimius · 2017-06-14T10:46:54Z

LGTM. Glad to have it in.

OlivierBlanvillain added the stat:wip label Jun 1, 2017

smarter reviewed Jun 1, 2017

View reviewed changes

DarkDimius reviewed Jun 2, 2017

View reviewed changes

OlivierBlanvillain added 5 commits June 7, 2017 10:09

Split Simplify.scala in 15 files

328357c

Add -Yopt-phases, -Yopt-fuel, bisect.sh, update main Simplify loop

e93b499

DropNoEffects: fix first bootstrap bug

257954c

DropNoEffects: comments and formating

0ed8e0a

DropNoEffects: Remove TypeApply case, this is covered by DropGoodCasts

9c152e8

OlivierBlanvillain force-pushed the localopt-continued branch from 6697282 to 9c152e8 Compare June 7, 2017 11:32

DarkDimius reviewed Jun 7, 2017

View reviewed changes

smarter reviewed Jun 7, 2017

View reviewed changes

Typo: receiver → receiver

03c0ca6

smarter reviewed Jun 7, 2017

View reviewed changes

OlivierBlanvillain force-pushed the localopt-continued branch from bb0f005 to 16671f2 Compare June 7, 2017 14:25

Formatting, adressing smarter comments

f6e4632

OlivierBlanvillain force-pushed the localopt-continued branch from 16671f2 to f6e4632 Compare June 7, 2017 14:38

DarkDimius force-pushed the localopt-continued branch from f6e4632 to f8cf56f Compare June 11, 2017 11:15

Improve Simplify's documentation

d69cd13

DarkDimius and others added 5 commits June 12, 2017 14:42

Use getSimpleName

ba0caa8

Collapse ctx and localCtx together

93e1b6d

Guard fuel with a private final val

a386f63

InlineCaseIntrinsics: add .simplified to fix new-unfriendy types

ff91037

Clean up

195d5e6

OlivierBlanvillain force-pushed the localopt-continued branch from f8cf56f to 195d5e6 Compare June 12, 2017 13:02

OlivierBlanvillain added 2 commits June 13, 2017 09:58

Bootstrap with -optimise

45232ba

OlivierBlanvillain added stat:needs review and removed stat:wip labels Jun 14, 2017

OlivierBlanvillain changed the title ~~[WIP] Local optimisations round two~~ Local optimisations round two Jun 14, 2017

Revert moving definitions to Definitions

4be324e

DarkDimius merged commit 2520992 into scala:master Jun 14, 2017

allanrenucci deleted the localopt-continued branch December 14, 2017 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local optimisations round two #2647

Local optimisations round two #2647

OlivierBlanvillain commented Jun 1, 2017

smarter Jun 1, 2017

smarter Jun 1, 2017

OlivierBlanvillain Jun 14, 2017

DarkDimius Jun 2, 2017

DarkDimius commented Jun 2, 2017

DarkDimius commented Jun 2, 2017 •

edited

Loading

OlivierBlanvillain commented Jun 2, 2017 •

edited

Loading

OlivierBlanvillain commented Jun 7, 2017

DarkDimius commented Jun 7, 2017

OlivierBlanvillain commented Jun 7, 2017

DarkDimius Jun 7, 2017

smarter Jun 7, 2017

smarter Jun 7, 2017

smarter Jun 7, 2017

DarkDimius commented Jun 11, 2017 •

edited

Loading

smarter commented Jun 11, 2017

DarkDimius commented Jun 11, 2017

OlivierBlanvillain commented Jun 11, 2017

OlivierBlanvillain commented Jun 12, 2017 •

edited

Loading

DarkDimius commented Jun 14, 2017

DarkDimius commented Jun 14, 2017

Local optimisations round two #2647

Local optimisations round two #2647

Conversation

OlivierBlanvillain commented Jun 1, 2017

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

DarkDimius commented Jun 2, 2017

DarkDimius commented Jun 2, 2017 • edited Loading

OlivierBlanvillain commented Jun 2, 2017 • edited Loading

OlivierBlanvillain commented Jun 7, 2017

DarkDimius commented Jun 7, 2017

OlivierBlanvillain commented Jun 7, 2017

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

DarkDimius commented Jun 11, 2017 • edited Loading

smarter commented Jun 11, 2017

DarkDimius commented Jun 11, 2017

OlivierBlanvillain commented Jun 11, 2017

OlivierBlanvillain commented Jun 12, 2017 • edited Loading

DarkDimius commented Jun 14, 2017

DarkDimius commented Jun 14, 2017

DarkDimius commented Jun 2, 2017 •

edited

Loading

OlivierBlanvillain commented Jun 2, 2017 •

edited

Loading

DarkDimius commented Jun 11, 2017 •

edited

Loading

OlivierBlanvillain commented Jun 12, 2017 •

edited

Loading