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.

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

Inliner does not inline method implementation from parent trait #6024

Closed
scabug opened this issue Jul 4, 2012 · 5 comments
Closed

Inliner does not inline method implementation from parent trait #6024

scabug opened this issue Jul 4, 2012 · 5 comments

Comments

@scabug
Copy link

scabug commented Jul 4, 2012

Consider the following code:

trait Base {
  def foo(p: Int => Boolean): Boolean
}

trait Impl extends Base {
  def foo(p: Int => Boolean): Boolean = p(1)
}

trait ImplOptimized extends Impl {
  @inline
  override final def foo(p: Int => Boolean): Boolean = p(1)
}

class Foo extends Base with ImplOptimized

class Bar {
  def bar(f: Foo) = f.foo(x => x == 1)
}

When compiled with -optimize -Yinline-warnings I get the following:
Test.scala:17: warning: Could not inline required method foo because callee (ImplOptimized.foo) has no code.
def bar(f: Foo) = f.foo(x => x == 1)

I asked Iulian about this and he said it might be tricky but should be possible to get inlining to work here. First we would need inline the forwarder Foo.foo that forwards to ImplOptimized.foo and then inline that method (with little twist related to handling $this).

Now, why getting this to work is important? Because we could mark all methods in LinearSeqOptimized as final and @inline and get rid of closure allocations for many operations on collections like List. This would be huge gain for the compiler.

@scabug
Copy link
Author

scabug commented Jul 4, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6024?orig=1
Reporter: @gkossakowski
Assignee: @magarciaEPFL
Affected Versions: 2.10.0
Duplicates #4767

@scabug
Copy link
Author

scabug commented Jul 4, 2012

@hubertp said:
Somehow related to SI-4767?

@scabug
Copy link
Author

scabug commented Jul 4, 2012

@adriaanm said:
As Hubert said: duplicate.

@scabug scabug closed this as completed Jul 4, 2012
@scabug
Copy link
Author

scabug commented Jul 4, 2012

@gkossakowski said:
Sorry, I somehow missed that one.

@scabug
Copy link
Author

scabug commented Jul 4, 2012

@paulp said:
But good to know you performance nose is working, because #4767 has long been high on my list of things which will make a difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant