Add/elim static this #624
Closed
Add/elim static this #624
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Constants that are adapted to a different supertype need to do this explicitly (not just by changing the type). Otherwise tree checkers will compute the original type and fail. This caused a test failure in pos/harmonize. The mystery is why this was not caught in the checkin tests.
Previously was only done for DefDefs. Caused backend failure.
|
we are experiencing a silend failure of VM here. |
|
@DarkDimius I haven't seen that failure mode before. Would be interested to see what it minimizes to. |
|
@DarkDimius : are you sure the interpreter reaches |
|
@smarter, I'm not sure what's happening here, to be honest. What I see, is that when running this test, we have a infinitely running java process, which has no code compiled by JIT, which has no monitors grabbed, all threads are in state runnable, and uses 0% cpu. This really feels like JVM stops in interpreter and just sleeps forever. If you have any clues, I'd welcome them). |
|
OK, I've managed to reduce this as much as I could. import scala.collection.parallel.immutable.ParVector
object Test {
val f: Int => Int = x => x + 1
val pv = ParVector(1, 2)
println("A")
pv.map(f) // Comment this line to avoid the deadlock
println("B")
def main(args: Array[String]): Unit = {
println("C")
}
}In Dotty, this will print "A" then get stuck. In scalac 2.11.6, it will work correctly by default, but if you use import scala.runtime.AbstractFunction1
import scala.collection.parallel.immutable.ParVector
object Test {
def something: Int = 42
val f: Int => Int = new AbstractFunction1[Int, Int] {
def apply(x: Int): Int = {
println("before calling something")
something
println("after calling something")
0
}
}
val pv = ParVector(1, 2)
println("A")
pv.map(f) // Comment this line to avoid the deadlock
println("B")
def main(args: Array[String]): Unit = {
println("C")
}
}With both compiler it prints: A
before calling something
before calling somethingThen gets stuck. I think that the following happens at the bytecode level:
public final class Test {
public static void main(java.lang.String[]);
Code:
0: getstatic #16 // Field Test$.MODULE$:LTest$;
public static {};
Code:
0: new #2 // class Test$
3: invokespecial #12 // Method "<init>":()V
6: return
public int apply$mcII$sp(int);
Code:
0: getstatic #22 // Field scala/Predef$.MODULE$:Lscala/Predef$;
3: ldc #24 // String before calling something
5: invokevirtual #28 // Method scala/Predef$.println:(Ljava/lang/Object;)V
8: getstatic #31 // Field Test$.MODULE$:LTest$;
11: invokevirtual #35 // Method Test$.something:()I
14: pop
15: getstatic #22 // Field scala/Predef$.MODULE$:Lscala/Predef$;
18: ldc #37 // String after calling something
20: invokevirtual #28 // Method scala/Predef$.println:(Ljava/lang/Object;)V
23: iconst_0
24: ireturn
|
|
This can be trivially worked around by calling |
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
May 30, 2015
See scala#624 (comment) for a lengthy explanation.
|
@smarter, good catch. Indeed this looks like classloading order problem. Interesting though, this deadlock cannot be seen from jvisualvm or jstack, as they say that none of threads own any locks. |
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
May 30, 2015
See scala#624 (comment) for a lengthy explanation.
|
OK, pushed there also. |
|
I guess this wouldn't be the last test failing, there are plenty of run-tests that either use multithreading or parallel collections(github finds 10 pages of references to |
|
I think that the more systematic fix would be to implement |
|
Note that this is actual difference in behavior with respect to scalac: by maing lambda implementation static, we do not capture For normal classes, not capturing The simple solution that I see now, is to make lambda implemetations static in Test, not in Test$. This will fix the deadlock. |
|
@smarter This is not specific to DelayedInit at all, this is a behavioral difference for all globally visible objects. |
|
Yes, but if you use |
|
@smarter, code that you wrote above is perfectly valid scala code. It should work out-of-the box. The fact that we make some methods static, and by this we could introduce a deadlock I would consider an implementation detail. As an implementation detail, this should be transparent to user. |
|
Yes, I agree that a solution that avoids all deadlock would be even better, but that seems harder :). |
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
May 30, 2015
See scala#624 (comment) for a lengthy explanation.
See scala#624 (comment) for a lengthy explanation.
smarter
force-pushed
the
add/elim-static-this
branch
from
2c60e0a to
fc1d8e6
Compare
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
May 30, 2015
See scala#624 (comment) for a lengthy explanation.
|
@smarter Great analysis! @DarkDimius Yes, I think we should put static functions in the companion class, not the module class. |
|
This PR should be closed, it is superseded by #623 (I don't have the rights to close it myself) |
odersky
added a commit
to dotty-staging/dotty
that referenced
this pull request
Jun 1, 2015
See scala#624 (comment) for a lengthy explanation. We now solve the problem in LambdaLift. The formerly failing tests are all reverted to theor original, failing, version.
Another advantage is that the lambda can be serializable, even if the enclosing module class is not. We are working on similar issues in scala/scala#5099 |
This confused me when I first saw it. The trick is to know that
|
|
@retronym, note that code written in this PR had a bug that caused most methods not to emitted as static. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For what it's worth, here's a phase to eliminate This(...) from static methods. Not sure whether it does anything useful. Impossible to say without tests working. @DarkDimius maybe it's useful for your fixes.