Rework tree transforms #3366

odersky · 2017-10-22T20:14:43Z

Based on #3350.

odersky · 2017-10-22T20:15:01Z

test performance please

dottybot · 2017-10-22T20:15:55Z

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

dottybot · 2017-10-22T21:17:30Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (0438e48)

odersky · 2017-10-23T12:12:01Z

test performance please

dottybot · 2017-10-23T12:12:41Z

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

dottybot · 2017-10-23T14:46:51Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (e1365d9)

odersky · 2017-10-23T14:49:00Z

test performance please

dottybot · 2017-10-23T14:49:15Z

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

dottybot · 2017-10-23T15:51:42Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (e1365d9)

smarter · 2017-10-23T16:15:55Z

Could you give a list of differences between TreeTransformer and SuperPhase?

smarter · 2017-10-23T16:20:00Z

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

+ *  This is an evolution of the previous "TreeTransformers.scala", which was written by @DarkDimius and
+ *  is described in his thesis.
+ */
+object SuperPhase {


Bikeshed: I'm not a big fan of the name SuperPhase since super already has a very specific meaning in Scala. Some possible alternatives: MegaPhase,FusedPhase, JoinedPhase.

OK, let's go with MegaPhase. I like the sound of that :-)

smarter · 2017-10-23T16:20:19Z

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

+    }
+
+    { implicit val ctx = nestedCtx
+      tag match {


This could be tag: @switch

smarter · 2017-10-23T16:25:41Z

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

+
+  /** Transform full tree using all phases in this group that have idxInGroup >= start */
+  def transformTree(tree: Tree, start: Int)(implicit ctx: Context): Tree = {
+    val tag = tree.tag


We could avoid the megamorphic dispatch here if we had something like:

abstract class Tree[-T >: Untyped](val tag: TreeTag) ... // ... class Ident(...) extends Tree(Tag.Ident) with ...

Yes, but at the cost of one field per tree. It might still be worth it, though. One needs to experiment.

Do we have benchmarks now that support this optimization?

DarkDimius · 2017-10-23T16:27:37Z

I should be able to review this on Wednesday.
Would you kindly give me a short summary of what motivated the changes in last 3 commits? It will give a perspective for review.

odersky · 2017-10-24T12:22:53Z

The motivation from the change came from me trying to understand (again) TreeTransformers.scala when I did the work on running all transforms at group end. I noted that things were very convoluted, and repetitive and that, as a consequence it was hard to find the right way to do changes. For instance there was the issue that TreeTransforms, but not TeeTransformers used cpyBetweenPhases. I believe that was not intentional (or was it?), but rather caused by the fact that due to its length, TreeTransformers is hard to maintain. The new set of changes cuts the size of TreeTransformers to roughly half of what it was before and clearly separates actual implementation logic (~200loc) from initialization code.

There was also the motivation to play with the implementation a bit to see whether we could wring more performance out of it. To be able to judge performance, I believe we have to get #3350 in first and then compare. I am not yet sure performance is better or worse. But to play with it I felt it was necessary to simplify it first.

In detail, the changes can be summarized as follows:

MegaPhase is a simplified version of TreeTransformer

about half of the size
no need for separate TransformerInfo arguments
simpler wiring logic

The simplifications are largely due to three changes

use of function values, where wiring is done by a sort of staging
use of tree tags, which give a common index into tables of transformer lambdas
prepare... operations return a Context instead of a TreeTransform, so there are
fewer "moving parts".

odersky · 2017-10-24T12:24:08Z

For both review and performance comparisons it would be best if we got #3350 in first.

liufengyun

There is another base class MacroTransform in the code, which is only used once. I think it's not justified to have a separate concept for it. Is it possible to get rid of it as well?

liufengyun · 2017-10-24T16:04:14Z

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

+  def newLocation[T](): (Location[T], Store) = {
+    val elems1 = new Array[AnyRef](elems.length + 1)
+    System.arraycopy(elems, 0, elems1, 0, elems.length)
+    (new Location(elems.length), new Store(elems1))


For another PR: minor optimisations here to double the size of array and keep track of size?

That would be complicated because arrays are purely functional. I.e you have to copy anyway on an update.

nicolasstucki · 2017-10-25T14:17:47Z

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

+
+    final val TypedSplice = 40
+    final val ModuleDef = 41
+    final val ParsedTry = 42


I would add a comment describing which tag numbers can be reused and why.

👍 It's not clear for me why are they reused.

There's no re-use: NumTypeTreeTags is a counter, not a tag. I'll add a comment to make that clearer.

DarkDimius · 2017-10-26T05:56:00Z

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

+  type TreeTag = Int
+
+  object Tag {
+    final val Ident = 0


We can use the same tags that are used in tasty. WDYT?

We could try, but then the switches will not be dense anymore, so will most likely not be implemented by a table switch.

Also, in that case unpickled trees would not have tags, so we could not use tags for e.g. typer.

DarkDimius · 2017-10-26T05:56:25Z

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

@@ -357,6 +425,7 @@ object Trees {
  case class Ident[-T >: Untyped] private[ast] (name: Name)
    extends RefTree[T] {
    type ThisTree[-T >: Untyped] = Ident[T]
+    def tag = Tag.Ident


DarkDimius · 2017-10-26T05:58:25Z

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

+
+  /** Transform full tree using all phases in this group that have idxInGroup >= start */
+  def transformTree(tree: Tree, start: Int)(implicit ctx: Context): Tree = {
+    val tag = tree.tag


Do we have benchmarks now that support this optimization?

DarkDimius · 2017-10-26T05:59:55Z

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

+            val rhs = transformTree(tree1.rhs, start)
+            trans(cpy.DefDef(tree1)(tree1.name, tparams, vparamss, tpt, rhs))
+          }
+          transformDefDef(localContext)


If we're trying to get all possible performance here - localContext has extra branch that DefDef never needs.

DarkDimius · 2017-10-26T06:02:38Z

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

+      else {
+        var clsMethods = clsMethodsCache.get(cls)
+        if (clsMethods eq null) {
+          clsMethods = cls.getDeclaredMethods


proposal: add map(_.getName) here and use contains instead of exists below.

DarkDimius · 2017-10-26T06:03:33Z

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

+  private def newTransformerArray[T, R](
+      methName: String, elemFn: IndexedTransformer[T, R], last: Transformer[T, R]) = {
+    val trans = new Array[Transformer[T, R]](miniPhases.length + 1)
+    trans(miniPhases.length) = last


:-) Now it's actually closer to version described in the thesis.

DarkDimius · 2017-10-26T06:04:51Z

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

+  private val nodeTransformer: Array[Array[Transformer[Tree, Tree]]] =
+    Array.fill(Tag.NumTags)(otherNodeTransformer)
+
+  private def init(name: String, tag: TreeTag, ctf: IndexedTransformer[_, Context], ntf: IndexedTransformer[_, Tree]): Unit = {


I think init is too generic name and also counterintuitive given that it is called more than once.

DarkDimius · 2017-10-26T06:06:23Z

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

+
+    final val TypedSplice = 40
+    final val ModuleDef = 41
+    final val ParsedTry = 42


👍 It's not clear for me why are they reused.

DarkDimius · 2017-10-26T06:10:48Z

I like the idea of stores inside Context instead of mutable state inside phases.

DarkDimius · 2017-10-26T06:13:00Z

I've left comments on individual commits.

I'm very curious to see what impact on performance will manual handling of tags have compared to instanceOf tests.

SuperPhase is a simplified version of TreeTransformer - about half of the size - no need for separate TransformerInfo arguments - simpler wiring logic The simplifications are largely due to two changes - use of function values, where wiring is done by a sort of staging - prepare... operations return a Context instead of a TreeTranform

... and provide a way to initialize contexts when phases are linked.

Convert phases that created new TreeTransforms before to use the new Store concept instead of Properties.

and add @switch annotation to core match in it.

The switch on tags in transformTree was a lookup switch with the previous distribution of tags. By moving tags that get eliminated before pickling to the end we can turn it into a table switch.

odersky · 2017-10-26T09:32:30Z

test performance please

dottybot · 2017-10-26T09:32:36Z

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

Mixin changes the number of parameters of trait initializers and therefore needs relaxed typing. This was not discovered before because TreeTransformers did not use a time travelling tree copier.

dottybot · 2017-10-26T14:07:35Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (07e4968)

scala#3366 did lose some performance relative to the previous tree transform scheme. http://dotty-bench.epfl.ch/3366/ shows a loss of about 4% for scalapb and dotty. This commit is trying to get it back, using two tweaks: - We don't call an empty context or node transformer, instead we represent such transformers now by null and return immediately if we hit a null transformer - After a transform, we do a test whether we still have a tree of the same tag before computing the treeTag of the transformed tree. This avoids the megamorphic dispatch in the common case where the tag is the same.

allanrenucci requested review from DarkDimius and liufengyun October 23, 2017 10:11

smarter reviewed Oct 23, 2017

View reviewed changes

odersky mentioned this pull request Oct 24, 2017

Make rarely updated context fields elements of the store #3375

Merged

odersky force-pushed the change-transform-phases-4 branch from a51f0fb to e73974a Compare October 24, 2017 12:58

liufengyun approved these changes Oct 24, 2017

View reviewed changes

nicolasstucki reviewed Oct 25, 2017

View reviewed changes

nicolasstucki approved these changes Oct 25, 2017

View reviewed changes

DarkDimius reviewed Oct 26, 2017

View reviewed changes

odersky mentioned this pull request Oct 26, 2017

Run all miniphase transforms at group end #3350

Merged

odersky added 3 commits October 26, 2017 11:26

Add tag method to Tree nodes

0934483

Introduce SuperPhase

681ea26

odersky added 7 commits October 26, 2017 11:27

Fix test script

568a8d3

Add Store to Concept

971c54f

... and provide a way to initialize contexts when phases are linked.

Use Store in MiniPhases

c5b1201

Convert phases that created new TreeTransforms before to use the new Store concept instead of Properties.

Convert more phases to use stores

61a60e8

Rename SuperPhase -> MegaPhase

cf93f4a

and add @switch annotation to core match in it.

Reorder TreeTags

19de54f

The switch on tags in transformTree was a lookup switch with the previous distribution of tags. By moving tags that get eliminated before pickling to the end we can turn it into a table switch.

Fix rebase breakage

0bd23f8

odersky force-pushed the change-transform-phases-4 branch from e73974a to 0bd23f8 Compare October 26, 2017 09:31

odersky added 2 commits October 26, 2017 12:08

Bring back relaxedTyping

346f1f2

Make Mixin a relaxedTyping phase

5a41b3e

Mixin changes the number of parameters of trait initializers and therefore needs relaxed typing. This was not discovered before because TreeTransformers did not use a time travelling tree copier.

odersky force-pushed the change-transform-phases-4 branch from 3a8cde2 to 5a41b3e Compare October 26, 2017 12:40

odersky merged commit 02252a8 into scala:master Oct 26, 2017

odersky mentioned this pull request Oct 26, 2017

Optimize MegaPhase #3393

Merged

allanrenucci deleted the change-transform-phases-4 branch December 14, 2017 16:58

Rework tree transforms #3366

Rework tree transforms #3366

Uh oh!

Conversation

odersky commented Oct 22, 2017

Uh oh!

odersky commented Oct 22, 2017

Uh oh!

dottybot commented Oct 22, 2017

Uh oh!

dottybot commented Oct 22, 2017

Uh oh!

odersky commented Oct 23, 2017

Uh oh!

dottybot commented Oct 23, 2017

Uh oh!

dottybot commented Oct 23, 2017

Uh oh!

odersky commented Oct 23, 2017

Uh oh!

dottybot commented Oct 23, 2017

Uh oh!

dottybot commented Oct 23, 2017

Uh oh!

smarter commented Oct 23, 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!

smarter Oct 23, 2017 • 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!

DarkDimius commented Oct 23, 2017

Uh oh!

odersky commented Oct 24, 2017

Uh oh!

odersky commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

odersky Oct 26, 2017 • 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!

smarter Oct 23, 2017 •

edited

Loading

odersky commented Oct 24, 2017 •

edited

Loading

odersky Oct 26, 2017 •

edited

Loading