Specialize Functions #1807

felixmulder · 2016-12-15T14:35:10Z

This PR is a bit of a work in progress, but I'd like feedback on the code from @DarkDimius and/or @odersky.

Todo:

Specialize unobscured Function1 subclasses
Test for presence of specialized apply
Test for absence of call to boxing runtime

PS: got stuck at Zurich airport, so haven't run the full test-suite yet. Letting our CI do the heavy lifting instead..

felixmulder · 2016-12-15T15:08:27Z

Perhaps it would be a good idea to move these phases to after erasure, since

class Func1 extends Function1[Int, Int] {
  def apply(i: Int) = i + 1
}

// this cast will be typed as `Int => Int` until erasure
(new Func1: Function1[Int, Int])(1)

and the transformation doesn't catch it currently.

DarkDimius · 2016-12-16T09:56:27Z

compiler/src/dotty/tools/dotc/Compiler.scala

-           new Splitter),           // Expand selections involving union types into conditionals
-      List(new VCInlineMethods,     // Inlines calls to value class methods
+           new Splitter,           // Expand selections involving union types into conditionals
+           new SpecializeFunction1), // Specialized Function1 by replacing super with specialized super


why do those two transformations have to be in different groups?

I was unsure of what impact the interleaved phases would have here. DispatchToSpecializedApply needs to know that the identifier type has a specialized apply before dispatching to it - so the addition to the decls (in SpecializeFunction1) needs to happen before this phase's transformation is applied.

Can they be the same phase? If so, what restriction on the transformation API am I missing?

DispatchToSpecializedApply needs to know that the identifier type has a specialized apply before dispatching to it

the specialized apply already exists in scala.Function1, you don't need to wait for them to be added.

tree tranformer runs after denotTransformer, so the new definitions will be already present in types.

DarkDimius · 2016-12-16T09:57:04Z

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

@@ -251,7 +251,7 @@ object NameOps {
      case nme.clone_ => nme.clone_
    }

-    def specializedFor(classTargs: List[Types.Type], classTargsNames: List[Name], methodTargs: List[Types.Type], methodTarsNames: List[Name])(implicit ctx: Context): name.ThisName = {
+    def specializedFor(classTargs: List[Types.Type], classTargsNames: List[Name], methodTargs: List[Types.Type] = scala.Nil, methodTarsNames: List[Name] = scala.Nil)(implicit ctx: Context): name.ThisName = {


I'd prefer not to provide default arguments here, as it's easy to misuse this method.

fair enough

DarkDimius · 2016-12-16T09:58:32Z

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

+    tree match {
+      case Apply(select @ Select(id, nme.apply), arg :: Nil) =>
+        val params = List(arg.tpe, tree.tpe)
+        val specializedApply = nme.apply.specializedFor(params, params.map(_.typeSymbol.name))


todo: this will pollute name table a lot. I would be nice to check if the name already exists before creating it, as if it doesn't, the specialization doesn't exist either.

should this check be done in specializedFor?

yes) would be nice to have an extra boolean arg that checks if it should force creation.

DarkDimius · 2016-12-16T09:59:11Z

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

+import Denotations._, SymDenotations._, Scopes._, StdNames._, NameOps._, Names._
+import ast.tpd
+
+class SpecializeFunction1 extends MiniPhaseTransform with DenotTransformer {


why only function1?

we agreed to start there :)

But I could make it generic, albeit we only have specializations for JFunction0, JFunction1 and JFunction2

DarkDimius · 2016-12-16T10:00:47Z

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

+
+    blacklisted = Set(
+      "scala.compat.java8.JFunction1",
+      "scala.runtime.AbstractFunction1",


why do you need to blacklist those 3? As those functions don't know the actual type parameters of parent class, they shoundn't be specialized.

I.e. Aren't they already caught by https://github.com/lampepfl/dotty/pull/1807/files#diff-3a80aa3cb499ce4bfec8753eabcb1796R74?

This was me being careful, I initially had some errors because of trying to apply the transformations to these, but the blacklist is never checked against in the current set of tests - as you pointed out they're caught by the earlier checks.

DarkDimius · 2016-12-16T10:35:24Z

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

+
+  def transformDenot(cref: ClassDenotation, t1: Type, r: Type, func1: Type)(implicit ctx: Context): SingleDenotation = {
+    val specializedFunction: TypeRef =
+      ctx.requiredClassRef(functionPkg ++ specializedName(functionName, t1, r))


requiredClassRef would fail compilation if there's no such class. I don't think it should be used here.

getClassIfDefined instead?

DarkDimius · 2016-12-16T10:38:20Z

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

+    cref.copySymDenotation(info = newInfo)
+  }
+
+  private def transformTree(tmpl: Template, func1: Tree, t1: Type, r: Type)(implicit ctx: Context) = {


I'd define it inline, as you use it once and the name is less informative then TransformTemplate.

I wanted to keep things apart so that it would be easier to get a quick overview of the workings of SpecializeFunction1, but perhaps it was taking it a bit far...

DarkDimius · 2016-12-16T10:38:58Z

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

+      ctx.requiredClassRef(functionPkg ++ specializedName(functionName, t1, r))
+
+    def replaceFunction1(in: List[TypeRef]): List[TypeRef] =
+      in.foldRight(List.empty[TypeRef]) { (tp, acc) =>


you are recreating a list, removing all advantages of structural sharing.

use mapConserve

DarkDimius · 2016-12-16T10:39:47Z

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

+      (if (func1 eq t) specializedFunc1 else t) :: trees
+    }
+
+    val body = tmpl.body.foldRight(List.empty[Tree]) {


use mapConserve

DarkDimius · 2016-12-16T10:41:15Z

compiler/test/dotty/tools/dotc/transform/SpecializeFunction1Tests.scala

+        .map(_.name)
+        .toList
+
+      assert(


it would be also nice to check that the actual implementation is in the right apply.
Ie, apply$mcII$sp isn't a forwarder to generic apply that unboxes.

DarkDimius · 2016-12-16T10:43:11Z

Perhaps it would be a good idea to move these phases to after erasure, since

I don't think so. After erasure the information is already lost, and I don't see why you cant translate this block now...

// this cast will be typed as Int => Int until erasure
(new Func1: Function1[Int, Int])(1)

Where is a cast here? There shound be no cast needed here at all.

felixmulder · 2016-12-16T11:06:00Z

The cast stops the optimization from applying, currently

DarkDimius · 2016-12-16T11:35:28Z

but why is cast there in the first place? There shouldn't be any casts needed.

felixmulder · 2016-12-16T13:18:57Z

Without the cast (as should be in normal code) - the optimization applies and all is good. With the cast the optimization doesn't kick in - so the question arises: should it?

felixmulder · 2016-12-16T13:19:28Z

So - you're totally right, there aren't any casts needed. But if there's a cast. The optimization doesn't apply.

felixmulder · 2017-02-15T16:14:49Z

@DarkDimius - changes discussed today incorporated into the PR.

DarkDimius · 2017-02-16T11:38:41Z

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

+  private[this] var retTypes: Set[Symbol] = _
+
+  override def prepareForUnit(tree: Tree)(implicit ctx: Context) = {
+    retTypes = Set(defn.UnitClass,


why not take primitive value classes from ctx.definitons?
Same with argTypes.

DarkDimius · 2017-02-16T11:39:55Z

compiler/test/dotty/tools/dotc/transform/SpecializeFunctionsTests.scala

+        .getOrElse(assert(false, "Could not find constructor for object `Test`"))
+    }
+  }
+}


it would be nice to have more run-tests that check if classes&lambdas inherit the right specialized functions using reflection.

DarkDimius · 2017-02-16T11:40:15Z

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

+
+  // Transformations -----------------------------------------------------------
+
+  /** Transforms all classes extending `Function1[-T1, +R]` so that


Outdated comment?

DarkDimius · 2017-02-16T11:41:20Z

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

+
+  // Extractors ----------------------------------------------------------------
+  private object ShouldTransformDenot {
+    def unapply(cref: ClassDenotation)(implicit ctx: Context): Option[Seq[SpecializationTarget]] =


I think it's very simple implementation that doesn't warrant pattern-matching that allocates options.
Additionally, it's used only once. I'd inline it.

If you want to keep it as a pattern, maybe consider using unapplySeq

DarkDimius · 2017-02-16T11:43:41Z

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

+          functionPkg ++ specializedName(functionName ++ arity, args, ret)
+        }
+
+        val specializedApply: Symbol = {


what if specializedParent doesn't exist?

DarkDimius · 2017-02-16T11:45:58Z

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

+    case cref @ ShouldTransformDenot(targets) => {
+      val specializedSymbols: Map[Symbol, (Symbol, Symbol)] = (for (SpecializationTarget(target, args, ret, original) <- targets) yield {
+        val arity = args.length
+        val specializedParent = ctx.getClassIfDefined {


shoulnd't this class always exist?
Wasn't it already extracted in targets?

DarkDimius · 2017-02-16T11:55:16Z

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

+      def replace(in: List[TypeRef]): List[TypeRef] =
+        in.map { tref =>
+          val sym = tref.symbol
+          specializedSymbols.get(sym).map { case (specializedParent, _) =>


I this this is a very inefficient implementation.

Instead of building a Seq[(Symbol, (Symbol, Symbol)] and then converting it and operating it,
you can use the fact that both specializedSymbols and parents maintain the same order and traverse them together.

This will allow for an implementation that doesn't allocate any tuples, doesn't convert collections and traverses collections only once.

DarkDimius · 2017-02-16T11:55:54Z

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

+
+      val ClassInfo(prefix, cls, parents, decls, info) = cref.classInfo
+      val newParents = replace(parents)
+      val newInfo = ClassInfo(prefix, cls, newParents, specializeApplys(decls), info)


I think you don't need power of entire DenotTransformer. InfoTransformer will do.

DarkDimius · 2017-02-16T11:56:21Z

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

+    */
+  override def transformTemplate(tree: Template)(implicit ctx: Context, info: TransformerInfo) =
+    tree match {
+      case tmpl @ ShouldTransformTree(targets) => {


DarkDimius · 2017-02-16T11:56:43Z

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

+  override def transformTemplate(tree: Template)(implicit ctx: Context, info: TransformerInfo) =
+    tree match {
+      case tmpl @ ShouldTransformTree(targets) => {
+        val symbolMap = (for ((tree, SpecializationTarget(target, args, ret, orig)) <- targets) yield {


same here, but even worse -> you flatten maps.

DarkDimius · 2017-02-20T11:54:20Z

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

+
+  def transformInfo(tp: Type, sym: Symbol)(implicit ctx: Context) = tp match {
+    case tp: ClassInfo if !sym.is(Flags.Package) && (tp.decls ne EmptyScope) => {
+      val newDecls = tp.decls.cloneScope


why clone scope when no changes are performed?

DarkDimius · 2017-02-20T12:07:36Z

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

+      case Apply(select @ Select(id, nme.apply), arg :: Nil) => {
+        val params = List(arg.tpe, tree.tpe)
+        val specializedApply = specializedName(nme.apply, params)
+        val hasOverridenSpecializedApply = id.tpe.decls.iterator.exists {


why iterate through all by hand instead of performing member lookup?
Why do you need to check if sym is override?

DarkDimius · 2017-02-20T12:07:53Z

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

+          sym => sym.is(Flags.Override) && (sym.name eq specializedApply)
+        }
+
+        if (hasOverridenSpecializedApply) tpd.Apply(tpd.Select(id, specializedApply), arg :: Nil)


what's wrong about dispatching to an abstract one?

smarter · 2017-02-21T17:21:51Z

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

+    )
+    func2 = defn.FunctionClass(2)
+    func2Applys = List(
+      specApply(func2, defn.UnitType,    List(defn.IntType, defn.IntType)),


Please use loops 😭

…rmer`

When a class directly extends a specialized function class, we need to replace the parent with the specialized interface. In other cases we don't replace it, even if the parent of a parent has a specialized apply - the symbols would propagate anyway.

Duhemm · 2017-10-11T13:19:07Z

Superseded by #3306

felixmulder added the area:transform label Dec 15, 2016

felixmulder requested a review from DarkDimius December 15, 2016 14:35

felixmulder added the stat:in progress label Dec 15, 2016

DarkDimius suggested changes Dec 16, 2016

View reviewed changes

felixmulder force-pushed the topic/specialize-func1 branch from 27afa90 to 44253d4 Compare December 23, 2016 15:47

felixmulder force-pushed the topic/specialize-func1 branch 3 times, most recently from 70cd9ba to 43fed4c Compare January 2, 2017 13:28

felixmulder changed the title ~~[WIP] Specialize Function1~~ Specialize Function1 Jan 24, 2017

felixmulder requested a review from DarkDimius January 24, 2017 11:27

felixmulder added stat:needs review and removed stat:in progress labels Jan 26, 2017

felixmulder force-pushed the topic/specialize-func1 branch 2 times, most recently from 95ad5ab to 591bc8d Compare February 15, 2017 16:14

felixmulder force-pushed the topic/specialize-func1 branch 2 times, most recently from 7db2613 to 7a40ed1 Compare February 16, 2017 11:35

felixmulder changed the title ~~Specialize Function1~~ Specialize Functions Feb 16, 2017

DarkDimius suggested changes Feb 16, 2017

View reviewed changes

felixmulder force-pushed the topic/specialize-func1 branch from 9c05e0b to 615070e Compare February 17, 2017 12:57

DarkDimius reviewed Feb 20, 2017

View reviewed changes

felixmulder force-pushed the topic/specialize-func1 branch 2 times, most recently from 9c6871d to 3df75d5 Compare February 21, 2017 17:16

smarter reviewed Feb 21, 2017

View reviewed changes

felixmulder added 18 commits May 8, 2017 14:32

Do transformations on symbol level too

2e35558

Refactor transformations to be more idiomatic

aa790fa

Add dispatch to specialized applys

b5fa2bf

Add forwarding method for generic case

a46ade9

Don't specialize Function1 tree when invalid to

8f3d2fd

Write test to check for specialized apply

7858540

Remove DispatchToSpecializedApply phase

8c0ecd1

SpecializeFunction1: don't roll over parents, use mapConserve

fd66d93

Rewrite to handle all specialized functions

281628f

Don't remove parents not being specialized

16a09dd

Add plain function tests to NameOps and Definitions

8aeaa83

Rewrite SpecializeFunctions from DenotTransformer to `InfoTransfo…

4a84b0c

…rmer`

Add MiniPhaseTransform to add specialized methods to FunctionN

5df2904

Add synthetic bridge when compiling FunctionN

a206d77

Fix ordering of specialized names and type parameterized apply

662631d

Make ThisName recursive on self.ThisName

ff4e8d5

Make sure specialized functions get the correct name

e17672b

felixmulder force-pushed the topic/specialize-func1 branch from b4f3c67 to e17672b Compare May 8, 2017 12:36

felixmulder added stat:on hold and removed stat:needs review labels May 15, 2017

felixmulder modified the milestones: 0.2 Tech Preview, 0.1 Tech Preview May 29, 2017

smarter modified the milestones: 0.3 Tech Preview, 0.2 Tech Preview Jul 11, 2017

allanrenucci modified the milestones: 0.4 Tech Preview, 0.5 Tech Preview Sep 25, 2017

Duhemm mentioned this pull request Oct 11, 2017

Specialize Functions #3306

Closed

Duhemm closed this Oct 11, 2017


		// Transformations -----------------------------------------------------------

		/** Transforms all classes extending `Function1[-T1, +R]` so that

Specialize Functions #1807

Specialize Functions #1807

Uh oh!

Conversation

felixmulder commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixmulder commented Dec 15, 2016

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!

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!

DarkDimius commented Dec 16, 2016

Uh oh!

felixmulder commented Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DarkDimius commented Dec 16, 2016

Uh oh!

felixmulder commented Dec 16, 2016

Uh oh!

felixmulder commented Dec 16, 2016

Uh oh!

felixmulder commented Feb 15, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixmulder commented Dec 15, 2016 •

edited

Loading

felixmulder commented Dec 16, 2016 •

edited

Loading