Emit mixin forwarders by default? #2563
Comments
|
@smarter, I still believe that the results of this blog-post are not true for jit-ed code and we should perform a more close study. |
|
While this may help startup time, but I feel this would only worsen peak performance |
|
scala/scala#5429 says:
And also:
|
|
Citing your citation:
I don't think we should make a decision here before having a clear picture. The picture that is desciben in the blogpost i believe is not what happens in JVM. And, to cite the update to this blogpost:
the 20% slowdown was due to different reason. |
|
I totally agree that the discussion in the blog post cannot be related to what happens to the compiler performance. However, the decision to merge scala/scala#5429 was not based on the blog post, but on the benchmarks shown in the PR discussion: scala/scala#5429 (comment) and later comments |
|
@lrytz, thanks for the link. I think that we should NOT do this, as this can hide correctness bugs. We should make sure that our predicates on when to emmit forwarders for correctess is exercised. |
Well, hopefully before we have a stable release because switching
What kind of "close study" do you have in mind? If we run http://github.com/dotty-staging/compiler-benchmark and see that mixin forwarders improve cold performance without impacting hot performance, would that be enough?
One easy way to work around this is to run the tests twice, with mixing forwarders on or off. |
|
Also, our startup time won't be affected much.
I think we should understand how it works, not try to guess it. @lrytz and scalac-team tried to do this in the linked PR, but studies of that kind are always hard to perform, due to debug versions of JVM behaving very differently performance-wise. The right discussion seems to be here: scala/scala-dev#243, we should instrument the optimized build of hotspot to see if our assumptions are correct.
While I agree, I don't recall triggering bugs by emitting more forwarders rather then less. |
Just like what we did for concat in scala#7696, we add a dummy parameter list to avoid signature clashes between mixin forwarders, but this change should be less controversial since it only affects a deprecated method. Note that this isn't actually needed to compile with Dotty master yet because we only emit mixin forwarders when required to, but we will soon switch to emitting them all the time like scalac does (scala/scala3#2563), therefore we need to get the fix in first (and most importantly in time for 2.13).
Just like what we did for concat in scala#7696, we add a dummy parameter list to avoid signature clashes between mixin forwarders, but this change should be less controversial since it only affects a deprecated method. Note that this isn't actually needed to compile with Dotty master yet because we only emit mixin forwarders when required to, but we will soon switch to emitting them all the time like scalac does (scala/scala3#2563), therefore we need to get the fix in first (and most importantly in time for 2.13).
Just like what we did for concat in scala#7696, we add a dummy parameter list to avoid signature clashes between mixin forwarders, but this change should be less controversial since it only affects a deprecated method. Note that this isn't actually needed to compile with Dotty master yet because we only emit mixin forwarders when required to, but we will soon switch to emitting them all the time like scalac does (scala/scala3#2563), therefore we need to get the fix in first (and most importantly in time for 2.13).
This is what Scala 2.12+ does for cold performance reasons (see scala#5928 for more details) and we should align ourselves with them when possible. About two years ago in scala#2563, Dmitry objected that we may not need to do that if we found another way to get performance back, or if newer JDKs improved the performance of default method resolution. It doesn't look like these things have happened so far (but there's some recent glimmer of hope here: https://bugs.openjdk.java.net/browse/JDK-8036580). Dmitry also said "I don't recall triggering bugs by emitting more forwarders rather then less.", but in fact since then I've found one case where the standard library failed to compile with extra forwarders causing name clashes, requiring a non-binary-compatible change: scala/scala@e3ef657. As of scala#6079 this is no longer a problem since we now emit mixin forwarders after erasure like scalac, but it still seems prudent to emit as many forwarders as scalac to catch potential name clash issues.
This is what Scala 2.12+ does for cold performance reasons (see scala#5928 for more details) and we should align ourselves with them when possible. About two years ago in scala#2563, Dmitry objected that we may not need to do that if we found another way to get performance back, or if newer JDKs improved the performance of default method resolution. It doesn't look like these things have happened so far (but there's some recent glimmer of hope here: https://bugs.openjdk.java.net/browse/JDK-8036580). Dmitry also said "I don't recall triggering bugs by emitting more forwarders rather then less.", but in fact since then I've found one case where the standard library failed to compile with extra forwarders causing name clashes, requiring a non-binary-compatible change: scala/scala@e3ef657. As of scala#6079 this is no longer a problem since we now emit mixin forwarders after erasure like scalac, but it still seems prudent to emit as many forwarders as scalac to catch potential name clash issues.
This is what Scala 2.12+ does for cold performance reasons (see scala#5928 for more details) and we should align ourselves with them when possible. About two years ago in scala#2563, Dmitry objected that we may not need to do that if we found another way to get performance back, or if newer JDKs improved the performance of default method resolution. It doesn't look like these things have happened so far (but there's some recent glimmer of hope here: https://bugs.openjdk.java.net/browse/JDK-8036580). Dmitry also said "I don't recall triggering bugs by emitting more forwarders rather then less.", but in fact since then I've found one case where the standard library failed to compile with extra forwarders causing name clashes, requiring a non-binary-compatible change: scala/scala@e3ef657. As of scala#6079 this is no longer a problem since we now emit mixin forwarders after erasure like scalac, but it still seems prudent to emit as many forwarders as scalac to catch potential name clash issues.
Fix #2563: Unconditionally emit mixin forwarders
This is what Scala 2.12 does, for startup performance reasons (http://www.scala-lang.org/blog/2016/07/08/trait-method-performance.html). Currently we're only doing it for JUnit-annotated methods (#2557).
/cc @DarkDimius
The text was updated successfully, but these errors were encountered: