Specialize Functions #3306

Duhemm · 2017-10-11T13:18:45Z

Supersedes #1807 by @felixmulder

What I changed compared to the previous version:

The previous version would change the parent of a class extending FunctionN to the specialised version, if it existed. I encountered several issues doing that, and my investigation led me to believe that doing so is not necessary in this particular case. From my reading of @dragos's thesis, §4.4.2 "Specialized inheritance", it appears that changing the parent to the specialised version is important to have the child inherit the specialised functions rather than the generic ones. My understanding is that this is not necessary in this particular case, please let me know if I am misunderstanding the issue.
The optimisation wouldn't apply on by-name parameters, because it would run before ElimByName. I didn't manage to shuffle the phases around without adding a new traversal.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

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

test performance please

dottybot · 2017-10-11T13:20:17Z

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

smarter · 2017-10-11T14:14:07Z

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

+            val typeParams = tp.cls.typeRef.baseType(func).argInfos
+            val interface  = specInterface(typeParams)
+
+            if (interface.exists && tp.decls.lookup(nme.apply).exists) {


Instead of creating a class symbol with specInterface and checking if it actually exists, you can use isSpecializableFunction like in https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/transform/FunctionalInterfaces.scala#L39

smarter · 2017-10-11T14:16:10Z

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

+          dt.vparamss.head.map(_.symbol.info)
+        )
+
+        val specializedApply = tree.symbol.enclosingClass.info.decls.lookup(specName)


Same thing here and in transformApply, you should only have to do the lookup if isSpecializableFunction returns true

smarter · 2017-10-11T14:16:51Z

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

+      case _ => tree
+    }
+
+  @inline private def specializedName(name: Name, args: List[Type])(implicit ctx: Context) =


I don't think there's any reason for this method and the next one to be marked @inline

smarter · 2017-10-11T14:17:45Z

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

+      r  <- List(UnitType, BooleanType, IntType, FloatType, LongType, DoubleType)
+      t1 <- List(IntType, LongType, DoubleType)
+      t2 <- List(IntType, LongType, DoubleType)
+    } yield specApply(func2, List(t1, t2), r)


This duplicates some logic from isSpecializableFunction

dottybot · 2017-10-11T14:23:57Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (b8d1e41)

smarter · 2017-10-11T15:22:47Z

Most of the performance cost is probably from splitting a phase group to add SpecializeFunctions. Would be great if we could figure out a way to avoid that.

Duhemm · 2017-10-28T15:31:53Z

I think I managed to re-unify the group I split 🎉

Duhemm · 2017-10-28T15:32:00Z

test performance please

dottybot · 2017-10-28T15:32:02Z

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

dottybot · 2017-10-28T15:33:10Z

performance test failed:

Error line number: 145

[check /data/workspace/bench/logs/pull-3306-10-28-17.33.out for more information]

liufengyun · 2017-10-28T18:51:48Z

test performance please

dottybot · 2017-10-28T18:52:11Z

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

dottybot · 2017-10-28T19:56:11Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (68cc7ee)

liufengyun · 2017-10-28T23:54:08Z

test performance please

(meta test for bench infrastructure change)

dottybot · 2017-10-28T23:54:14Z

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

liufengyun · 2017-10-29T08:08:45Z

test performance please

(meta test for bench infrastructure change)

dottybot · 2017-10-29T08:09:14Z

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

dottybot · 2017-10-29T09:36:08Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (68cc7ee)

Duhemm · 2017-11-06T08:35:49Z

test performance please

dottybot · 2017-11-06T08:35:56Z

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

dottybot · 2017-11-06T10:02:45Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (22ff8a4)

smarter · 2017-11-06T12:35:51Z

Looks like this PR still affects performance, even the empty file benchmark seems to get slower. It'd be good to try to figure out where the extra time is being spent and if we can optimize this.

- Stop immediately if the type doesn't derive from `Function{0,1,2}` - We don't need to check if any of the parents derives from `Function{0,1,2}`, we can just check if the type derives from it.

Duhemm · 2017-11-19T15:20:33Z

test performance please

dottybot · 2017-11-19T15:21:33Z

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

dottybot · 2017-11-19T16:49:31Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (635d668)

dottybot reviewed Oct 11, 2017

View reviewed changes

Duhemm mentioned this pull request Oct 11, 2017

Specialize Functions #1807

Closed

3 tasks

smarter reviewed Oct 11, 2017

View reviewed changes

allanrenucci added this to the 0.5 Tech Preview milestone Oct 17, 2017

Duhemm force-pushed the topic/specialize-func1-rebased branch from cd2766f to e916a67 Compare October 28, 2017 15:14

Duhemm force-pushed the topic/specialize-func1-rebased branch from f5250fe to 5ecc4aa Compare November 6, 2017 08:20

Add phases and initial replacement for super

caa0a6b

felixmulder and others added 14 commits November 19, 2017 14:16

Make ThisName recursive on self.ThisName

6e79132

Make sure specialized functions get the correct name

bcf83cf

Fix compilation errors

8b6bb63

Don't change parents in specializeFunctions

de7b5f4

Cleanup

aa3f588

Move type to JVM tag conversion to Definitions

b9187d2

Add more tests for function specialization

a607938

Pass by name should not introduce boxing

efdc516

Cleanup

654fb14

No specialization for Function3

ed751bd

Adapt to recent changes in transformers and phases

a1c7109

Un-split phase group that I previously split

16b881d

Address review comments

8851312

Optimize phase SpecializeFunctions

a32b06f

- Stop immediately if the type doesn't derive from `Function{0,1,2}` - We don't need to check if any of the parents derives from `Function{0,1,2}`, we can just check if the type derives from it.

Duhemm force-pushed the topic/specialize-func1-rebased branch from 5ecc4aa to a32b06f Compare November 19, 2017 14:34

allanrenucci modified the milestones: 0.5 Tech Preview, 0.6 Tech Preview Nov 30, 2017

allanrenucci added the stat:wip label Dec 18, 2017

allanrenucci modified the milestones: 0.6 Tech Preview, 0.7 Tech Preview Jan 18, 2018

OlivierBlanvillain assigned Duhemm Jan 18, 2018

allanrenucci removed this from the 0.7 Tech Preview milestone Jul 23, 2018

Duhemm added stat:revisit and removed stat:wip labels Nov 28, 2018

Duhemm closed this Nov 28, 2018

Duhemm deleted the topic/specialize-func1-rebased branch November 30, 2018 07:56

liufengyun mentioned this pull request Sep 22, 2020

Revive function specialization #9846

Closed

Specialize Functions #3306

Specialize Functions #3306

Uh oh!

Conversation

Duhemm commented Oct 11, 2017

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Uh oh!

Duhemm commented Oct 11, 2017

Uh oh!

dottybot commented Oct 11, 2017

Uh oh!

smarter Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

smarter Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

smarter Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

smarter Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

dottybot commented Oct 11, 2017

Uh oh!

smarter commented Oct 11, 2017

Uh oh!

Duhemm commented Oct 28, 2017

Uh oh!

Duhemm commented Oct 28, 2017

Uh oh!

dottybot commented Oct 28, 2017

Uh oh!

dottybot commented Oct 28, 2017

Uh oh!

liufengyun commented Oct 28, 2017

Uh oh!

dottybot commented Oct 28, 2017

Uh oh!

dottybot commented Oct 28, 2017

Uh oh!

liufengyun commented Oct 28, 2017

Uh oh!

dottybot commented Oct 28, 2017

Uh oh!

liufengyun commented Oct 29, 2017

Uh oh!

dottybot commented Oct 29, 2017

Uh oh!

dottybot commented Oct 29, 2017

Uh oh!

Duhemm commented Nov 6, 2017

Uh oh!

dottybot commented Nov 6, 2017

Uh oh!

dottybot commented Nov 6, 2017

Uh oh!

smarter commented Nov 6, 2017

Uh oh!

Duhemm commented Nov 19, 2017

Uh oh!

dottybot commented Nov 19, 2017

Uh oh!

dottybot commented Nov 19, 2017

Uh oh!

Uh oh!