Skip to content

Revive function specialization #9846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 36 commits into from
Closed

Revive function specialization #9846

wants to merge 36 commits into from

Conversation

liufengyun
Copy link
Contributor

Revive function specialization

Based on #3306

@liufengyun liufengyun marked this pull request as ready for review September 24, 2020 16:46
smarter
smarter reviewed Sep 28, 2020
View reviewed changes
@scala scala deleted a comment from dottybot Sep 28, 2020
@liufengyun liufengyun force-pushed the fun-spec branch 2 times, most recently from a73d29a to 674d375 Compare October 7, 2020 08:07
@sjrd
Copy link
Member

sjrd commented Oct 7, 2020

Could we entirely disable the two phases when compiling for Scala.js? Like this:
https://github.com/lampepfl/dotty/blob/99548bd15dc13fec1998aa7fb9eeca5cb772d354/compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala#L248-L249
but with the opposite test.

@liufengyun
Copy link
Contributor Author

Could we entirely disable the two phases when compiling for Scala.js? Like this:
https://github.com/lampepfl/dotty/blob/99548bd15dc13fec1998aa7fb9eeca5cb772d354/compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala#L248-L249

but with the opposite test.

It's done in a943064

@liufengyun liufengyun force-pushed the fun-spec branch 3 times, most recently from 04cfd2c to 30087ee Compare October 12, 2020 12:04
@liufengyun
Copy link
Contributor Author

Regarding the performance regression in the scalap project: the project has ~2KLOC, it defines 118 classes/traits, of which 68 are case classes. To include the auto-generated companion objects, there will be 186 classes in total.

All the 68 companion objects of the case classes extend Function1-N, thus they reach the specialization code.

@liufengyun liufengyun requested a review from smarter October 13, 2020 11:16
@smarter
Copy link
Member

smarter commented Oct 13, 2020

All the 68 companion objects of the case classes extend Function1-N, thus they reach the specialization code.

I'm still surprised this makes such a significant difference, especially since specialization should only be applicable for some of these case classes, do you know which part of the code is responsible for the slow-down, or is it spread across everything?

@liufengyun
Copy link
Contributor Author

All the 68 companion objects of the case classes extend Function1-N, thus they reach the specialization code.

I'm still surprised this makes such a significant difference, especially since specialization should only be applicable for some of these case classes, do you know which part of the code is responsible for the slow-down, or is it spread across everything?

My conjecture is SpecializeFunctions.transformInfo, which has to do complex semantic operations for each class that extends Function0-2. I'll try if we can get some information from profiling.

smarter
smarter requested changes Oct 13, 2020
View reviewed changes
@dottybot
Copy link
Member

dottybot commented Nov 9, 2020

performance test scheduled for 8c10e26 1e6cd26: 5 job(s) in queue, 1 running.

@dottybot
Copy link
Member

dottybot commented Nov 9, 2020

Performance test finished successfully:

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

Benchmarks is based on merging with master (819e0b1)

@liufengyun liufengyun force-pushed the fun-spec branch 2 times, most recently from f647585 to 6a48696 Compare November 10, 2020 12:07
@scala scala deleted a comment from dottybot Nov 10, 2020
@scala scala deleted a comment from dottybot Nov 10, 2020
@scala scala deleted a comment from dottybot Nov 10, 2020
@scala scala deleted a comment from dottybot Nov 10, 2020
@dottybot
Copy link
Member

Performance test finished successfully:

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

Benchmarks is based on merging with master (4c055c4)

@liufengyun
Copy link
Contributor Author

retest performance please: c524986 b6554e9 ae5a675

@dottybot
Copy link
Member

performance test scheduled for c524986 b6554e9 ae5a675: 7 job(s) in queue, 1 running.

@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
@scala scala deleted a comment from dottybot Nov 12, 2020
liufengyun and others added 5 commits November 12, 2020 10:42
@liufengyun
Add specialization phases
fccd3dd 
- Fix isPlainFunctionClass

Previous implementation is incorrect, as scala.Function1$ would qualify.

- Create the symbol at the next phase

Create the symbol at the next phase, so that it is a valid member of the
corresponding function for all valid periods of its SymDenotations.
Otherwise, the valid period will offset by 1, which causes a stale symbol
in compiling stdlib.

- Handle abstract apply and multiple applys

- Don't specialize abstract apply

- Fast specialization

We avoid going through InfoTransformer, which will cause all symbols to be checked.
The reason why it works is that the specialized base classes, i.e. Function0-2
already have all the relevant definitions.
@liufengyun
change 1
b33afd9
@liufengyun
change 2
d3fc1d6
@liufengyun
Performance tweeak
a8127bb 
- Fix typo

- Use StringBuilder instead of StringBuffer (thanks @smarter)

StringBuffer is synchronized thus is slower.
@nicolasstucki @liufengyun
Port Function0 signature change
1aa0d6e 
See https://github.com/scala/scala/pull/9307/files
@liufengyun
Copy link
Contributor Author

retest performance please: b33afd9 d3fc1d6 1aa0d6e

@dottybot
Copy link
Member

performance test scheduled for b33afd9 d3fc1d6 1aa0d6e: 3 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

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

Benchmarks is based on merging with master (c3ac180)

@liufengyun
Copy link
Contributor Author

From the latest benchmark result http://dotty-bench.epfl.ch/9846-10 , the regression in scalap is caused by the code in d3fc1d6 (which agrees with the local benchmarking result). After fixing one typo in a8127bb, which creates 54 fewer methods, the compiler becomes faster (StringBuffer might also help a little bit).

@liufengyun
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

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

@dottybot
Copy link
Member

Performance test finished successfully:

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

Benchmarks is based on merging with master (462a72f)

@liufengyun liufengyun mentioned this pull request Nov 23, 2020
Merged
@liufengyun
Copy link
Contributor Author

Superseded by #10452

@liufengyun liufengyun closed this Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter requested changes

Assignees

@smarter smarter

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@liufengyun @sjrd @smarter @dottybot @felixmulder @Duhemm @nicolasstucki