8000 Optimizer: avoid inlining of accidental SAM types by lrytz · Pull Request #5895 · scala/scala · GitHub
[go: up one dir, main page]
Skip to content

Optimizer: avoid inlining of accidental SAM types #5895

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
Merged
merged 1 commit into from
May 17, 2017
Merged

Optimizer: avoid inlining of accidental SAM types #5895

merged 1 commit into from
May 17, 2017

Conversation

lrytz
Copy link
Member
@lrytz lrytz commented May 9, 2017

Before, any Scala defined class that happened to have a SAM was considered
a function type for the inliner heuristics. This can lead to unexpected
inlining, large methods and degraded performance. Jason recently saw such a
case when adding an abstract method mapOver to Type:

def typed1(tree: Tree, mode: Mode, pt: Type): Tree = {
  def typedInAnyMode(tree: Tree): Tree = ...

  ... typedInAnyMode(tree) ...
}

Lambdalift adds a pt: Type parameter to typedInAnyMode, so the callsite
forwards the pt parameter. This tirggered inlining once Type was a SAM,
and method typed1 became very large.

Now we only consider scala.FunctionN and types annotated @FunctionalInterface
as SAM types.

@lrytz
Optimizer: avoid inlining of accidental SAM types
f0b7949 
Before, any Scala defined class that happened to have a SAM was considered
a function type for the inliner heuristics. This can lead to unexpected
inlining, large methods and degraded performance. Jason recently saw such a
case when adding an abstract method `mapOver` to `Type`:

```
def typed1(tree: Tree, mode: Mode, pt: Type): Tree = {
  def typedInAnyMode(tree: Tree): Tree = ...

  ... typedInAnyMode(tree) ...
}
```

Lambdalift adds a `pt: Type` parameter to `typedInAnyMode`, so the callsite
forwards the `pt` parameter. This tirggered inlining once `Type` was a SAM,
and method `typed1` became very large.

Now we only consider `scala.FunctionN` and types annotated `@FunctionalInterface`
as SAM types.
@scala-jenkins scala-jenkins added this to the 2.12.3 milestone May 9, 2017
@lrytz lrytz requested a review from retronym May 9, 2017 09:42
@lrytz
Copy link
Member Author
lrytz commented May 9, 2017

I quickly ran hot -p source=scalap on my laptop (I didn't have a separate machine at hand, so just a quick test) before and after this change, the results look the same.

@retronym
Copy link
Member

The divergence in what the front and back-end consider to be a SAM type is a bit unfortunate. But I think that the heuristic you've used here is better than that status quo to avoid surprising over-inlining.

@retronym retronym closed this May 17, 2017
@retronym retronym reopened this May 17, 2017
@lrytz lrytz merged commit b69ed17 into scala:2.12.x May 17, 2017
@lrytz lrytz deleted the noInlineAccidentalSam branch June 3, 2017 14:42
@retronym retronym mentioned this pull request Sep 13, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@retronym retronym retronym approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.12.3
Development

Successfully merging this pull request may close these issues.
3 participants
@lrytz @retronym @scala-jenkins
0