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
Methods defined in traits are not inlined #4767
Comments
Imported From: https://issues.scala-lang.org/browse/SI-4767?orig=1 |
@paulp said: class PathResolver(settings: Settings, context: JavaContext) {
def this(settings: Settings) = this(settings, if (settings.inline.value) new JavaContext else DefaultJavaContext)
|
@paulp said: |
@paulp said: |
@TiarkRompf said: |
@paulp said: So before I continue any more down this road, since it feels like rather a lot of "original research" for something I had thought was closer to working order, I thought I'd ask for a sanity check. |
@TiarkRompf said: However it seems like Also, I may be missing something (a lot, possibly) but to me it looks like the lookup logic could be simplified into something like this: // look up methSym for a receiver instance of classSym:
var actualClass = classSym
var actualMeth = methSym
while (actualMeth.owner != actualClass) {
val ov = actualMeth.overridingSymbol(actualClass)
if (ov != NoSymbol) {
actualMeth = ov
assert(actualMeth.owner == actualClass)
} else {
actualClass = actualClass.superClass
}
}
val iclass = icodes.icode(actualClass,true)
val imethod = iclass.lookupMethod(actualMeth) |
@paulp said: GenICode: case DefDef(mods, name, tparams, vparamss, tpt, rhs) =>
debuglog("Entering method " + name)
val m = new IMethod(tree.symbol)
...
if (!m.isAbstractMethod && !m.native) {
// stuff which leads to m.code being populated
}
else
ctx1.method.setCode(null) I'll look around some more. |
@adriaanm said (edited on Jul 6, 2012 9:01:17 AM UTC): |
@magarciaEPFL said: |
@adriaanm said: |
@magarciaEPFL said: |
@adriaanm said (edited on Jul 6, 2012 2:59:01 PM UTC): |
@paulp said: |
@magarciaEPFL said: |
@paulp said: I threw some code at the TODO but am not 100% sure what DO equals so let me know if we're after something else. |
@adriaanm said (edited on Jul 9, 2012 9:43:34 AM UTC): Also, Miguel, thanks for looking into this. Traits are everywhere. So is a strong desire for inlining. |
@magarciaEPFL said: Still needs to be tested with the nightly. It would be great if @gkossakowski could gather data on its performance effect. |
@gkossakowski said: |
@magarciaEPFL said: |
@magarciaEPFL said: As a result:
In the example trait T {
@inline final def m(x: Int) {
println(x)
}
}
class C {
def host(y: T) {
y.m(7)
}
}
The resulting bytecode for public void host(T);
Code:
Stack=2, Locals=3, Args_size=2
0: bipush 7
2: istore_2
3: getstatic #15; //Field scala/Predef$.MODULE$:Lscala/Predef$;
6: iload_2
7: invokestatic #21; //Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
10: invokevirtual #25; //Method scala/Predef$.println:(Ljava/lang/Object;)V
13: return Without the @inline annotation, the bytecode for public void host(T);
Code:
Stack=2, Locals=2, Args_size=2
0: aload_1
1: bipush 7
3: invokestatic #15; //Method T$class.m:(LT;I)V
6: return With the current optimizer as we know the callsite to 3: invokeinterface #15, 2; //InterfaceMethod T.m:(I)V Here's how the new optimizer supports inlining of trait methods, documentation included: |
@adriaanm said: |
@adriaanm said: |
@adriaanm said: |
Malcolm Greaves (malcolmgreaves) said: |
@retronym said: See: scala/scala#4312 |
Compiling
yields (with -Ydebug and -Ylog:inline)
Expected behavior would be inlining
foo
intobar
. It all works if you changeFoo
to be a class instead of a trait.I did some digging around and apparently ICode reading does not resolve methods bodies in traits:
Furthermore, it looks like class file reading will never load a trait's impl class (which actually contains the method bodies):
There's really no way to get at it:
The reason is that
ClassPath.scala
explicitly excludes these class files (ending in$class.class
):I believe this is a bug on its own - commenting out
isValidName
fixes theimplClass
behavior but not the inlining issue altogether.The text was updated successfully, but these errors were encountered: