Skip to content
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

Support tuple specialisation #15060

Merged
merged 15 commits into from
May 30, 2022
Merged

Support tuple specialisation #15060

merged 15 commits into from
May 30, 2022

Conversation

dwijnand
Copy link
Member

No description provided.

@dwijnand dwijnand linked an issue Apr 28, 2022 that may be closed by this pull request
Support tuple specialization #11114
Closed
@dwijnand
Copy link
Member Author

Despite trying not to to start with, I ended up needing to allow for the parsing of specialised tuple (and product) classes, so that I could reach for the class and their constructor. Without that it ends up in a weird state where it gets removed as soon as you touch it ("implode-on-touch") due to, I expect, the deletion and markAbsent that using NoEmbedded leads to.

But now I realised I need to replace x._1() with x._1$mcI$sp() too, but the receiver is Tuple2 and now I can't find that method either - because I'm guessing it's in the bytecode and not the pickle... Getting to be quite the drag...

@sjrd
Copy link
Member

sjrd commented Apr 28, 2022

Could this be disabled when the -scalajs flag is on? In Scala.js, specialization does nothing but generate more code.

@dwijnand
Copy link
Member Author

Yep: override def isEnabled(using Context): Boolean = !ctx.settings.scalajs.value

@dwijnand dwijnand force-pushed the tuple-specialisation branch 3 times, most recently from 10f2f48 to b1bd1df Compare May 4, 2022 09:02
@dwijnand dwijnand force-pushed the tuple-specialisation branch from b1bd1df to e6042a5 Compare May 4, 2022 10:36
@dwijnand
Avoid crashing when compiling stdLib213
49e9d67 
    package scala does not have a member class Tuple2$mcCI$sp while compiling
    /__w/dotty/dotty/community-build/community-projects/stdLib213/src/library/scala/AnyValCompanion.scala,
    ...
    [error] dotty.tools.dotc.core.TypeError: package scala does not have a member class Tuple2$mcCI$sp
    [error] 	at dotty.tools.dotc.core.Denotations$Denotation.requiredSymbol(Denotations.scala:305)
    [error] 	at dotty.tools.dotc.core.Denotations$Denotation.requiredClass(Denotations.scala:340)
    [error] 	at dotty.tools.dotc.core.Definitions.SpecialisedTuple(Definitions.scala:1335)
    [error] 	at dotty.tools.dotc.transform.SpecializeTuples.transformApply(SpecializeTuples.scala:27)
@dwijnand dwijnand marked this pull request as ready for review May 4, 2022 17:38
@dwijnand dwijnand requested a review from odersky May 4, 2022 17:39
@dwijnand
Copy link
Member Author

dwijnand commented May 4, 2022

I was able to drop the hackaround I had in the classfile parser, so now it's just an exception in TreeChecker (-Ycheck). Compiling the standard library didn't work, for reasons that weren't clear, but it seemed niche so I just found a simple way to deal with it.

@dwijnand dwijnand added this to the 3.2.0-RC1 milestone May 12, 2022
@dwijnand
Copy link
Member Author

@sjrd any chance you could help me land this? I worked on it because it was desired for 3.2, and I believe we want to cut the RC1 for that next Tuesday (and Martin is traveling atm).

@sjrd
Copy link
Member

sjrd commented May 12, 2022

What is it blocked on? / How would you like me to help?

@dwijnand
Copy link
Member Author

@sjrd

What is it blocked on? / How would you like me to help?

It needs a review to be merged.

dwijnand added 5 commits May 16, 2022 16:01
@dwijnand
Add a "no-boxing" test for specialised tuples
1d2c641
@dwijnand
Match the original apply method name in tuple specialisation
434f7a7 
... not the new name from the import.
@dwijnand
Expose, tweak & use isElideableExpr in tuple specialisation
573a0c5 
Handle the contrived example constructing a non-elideable construction
of a tuple.  Requires exposing isElideableExpr, tweaking its
"isCaseClassApply" (to disallow block select qualifiers - while still
allowing bare ident applies), also fixing the StableRealizable flag on
tuple constructors so that isPureApply is true for those, so they can
elideable too.
@dwijnand
Switch isSpecializableTuple to classSymbol
599b8f3
@dwijnand
Fix defn.isSpecializedTuple/SpecialisedTuple with Context
fb11113
@dwijnand dwijnand requested a review from sjrd May 20, 2022 22:55
sjrd
sjrd approved these changes May 25, 2022
View reviewed changes
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Sorry for the delay.

odersky
odersky requested changes May 26, 2022
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good otherwise.

@odersky odersky assigned dwijnand and unassigned sjrd and odersky May 26, 2022
@dwijnand dwijnand assigned odersky and unassigned dwijnand May 30, 2022
odersky
odersky approved these changes May 30, 2022
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to get this in!

@odersky odersky merged commit 591db4d into scala:main May 30, 2022
@dwijnand dwijnand deleted the tuple-specialisation branch May 30, 2022 16:26
@SethTisue
Copy link
Member

SethTisue commented Sep 3, 2022 •
edited
Loading

Curious where/how it was decided to stop at arity 2?

(I wouldn't necessarily expect it to go up real high, like 22, but I guess I'd expect it to support the sizes that I have used in practice, which is up to about, idk, 5? At least 3, anyway?)

@dwijnand
Copy link
Member Author

dwijnand commented Sep 5, 2022

It was decided in the standard library. Tuple1 has 3 specialised classes: Double, Int and Long. Tuple2 has 25! (Int, Long, Double, Char, Boolean) x (Int, Long, Double, Char, Boolean)...

@Adam-Vandervorst
Copy link

Adam-Vandervorst commented Sep 7, 2022 •
edited
Loading

You're talking about the maximum number of specialised classes right @dwijnand?
The examples use known types (e.g. foo: (Int, Int)) for which I assume only one class is generated.
Even in many generic instances, the tuple types are not independent (e.g. pos: (N, N, N)) and only need 5 specializations.
In some other contexts, I can see it being useful to deduce (e.g. when N <: Int | Long) or manually specify the cases (e.g. when N: Integral).

@Kordyjan Kordyjan modified the milestones: 3.2.0-RC1, 3.2.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolasstucki nicolasstucki nicolasstucki left review comments

@sjrd sjrd sjrd approved these changes

@odersky odersky odersky approved these changes

Assignees

@odersky odersky

Labels
None yet
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.

Support tuple specialization
7 participants
@dwijnand @sjrd @SethTisue @Adam-Vandervorst @odersky @nicolasstucki @Kordyjan