Skip to content

Local optimisation: Inline local objects #2813

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.

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

Merged
merged 6 commits into from
Jul 10, 2017

Conversation

OlivierBlanvillain
Copy link
Contributor

@DarkDimius This is my take on re-implemented InlineLocalObjects. Why I have now is much simpler, and also does way less. In particular there is no special handling of labeldefs. Could you try coming up with unit test size examples showing up what's lost? I played around with the example from this gist, but generate the same byte-code than linker looks impossible without global program knowledge.

@DarkDimius
Copy link
Contributor

@OlivierBlanvillain, the gist you've referred to was optimized without global program knowledge.
Simplifier from https://github.com/dotty-linker/dotty/tree/opto generated the code in the gist.

@DarkDimius
Copy link
Contributor

Here are several examples of code that original InlineLocalObjects can handle but this reimplementation can not:

object Tupples{
  def foo(x: Boolean): Int = {
    val (a, b) = if (x) (1, 2) else (2, 3) // handling of IFs is missing
    a + b
  }
 def foo(x: Int): Int = {
    val (a, b) = x match {
        case i if (i%3 == 0) => (i, i + 1)
        case i if (i%2 == 0) => (i, i + 2)
        case _ => (0, 0)
      } // handling of LabelDefs is missing
    a + b
  }
}

While this is a nice re-implementation, it does not seem to be able to optimize even simple cases.
@OlivierBlanvillain, did you try it on any non-trivial examples that simply call constructor directly?

Copy link
Contributor

@DarkDimius DarkDimius left a comment

Choose a reason for hiding this comment

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

See above

Copy link
Contributor

@DarkDimius DarkDimius left a comment

Choose a reason for hiding this comment

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

This is improvement over the current state, I propose we get it in.

@DarkDimius DarkDimius merged commit b791979 into scala:master Jul 10, 2017
@allanrenucci allanrenucci deleted the inline-local-objects branch December 14, 2017 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants