Move mixin forwarders generation after erasure #6079
Merged
Conversation
This file contains 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
smarter
force-pushed
the
mixin-after-erasure-3
branch
2 times, most recently
from
781f18f to
6eefc75
Compare
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
smarter
added a commit
to smarter/scala
that referenced
this pull request
Mar 12, 2019
Since scala/scala3#6079, Dotty generates mixin forwarders after erasure just like Scala 2, so they don't cause extra name clashes, therefore we can revert 4366332 and e3ef657.
smarter
added a commit
to smarter/scala
that referenced
this pull request
Mar 12, 2019
Since scala/scala3#6079, Dotty generates mixin forwarders after erasure just like Scala 2, so they don't cause extra name clashes, therefore we can revert 4366332 and e3ef657.
|
also /cc @retronym since in scala/bug#8293 you said "Arguably, this was a sign that we ought to run mixin before erasure.", I think a149aed demonstrates that this is more trouble than its worth (hence why I ended up doing scala/scala#7843). |
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Mar 12, 2019
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.
smarter
force-pushed
the
mixin-after-erasure-3
branch
from
959ab41 to
ab689a4
Compare
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Mar 12, 2019
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.
smarter
force-pushed
the
mixin-after-erasure-3
branch
from
ab689a4 to
6eefc75
Compare
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/6079/ to see the changes. Benchmarks is based on merging with master (cb0d5f1) |
|
Should be available soon: http://dotty-bench.epfl.ch/6079 |
|
Huh, scala-library (which is a frozen version of the 2.12 stdlib) got 20% faster (and everything else stays the same) |
odersky
previously requested changes
Mar 17, 2019
compiler/src/dotty/tools/dotc/transform/ElimErasedValueType.scala
Outdated
Show resolved
Hide resolved
compiler/src/dotty/tools/dotc/transform/ElimErasedValueType.scala
Outdated
Show resolved
Hide resolved
And demonstrate that the current scheme of generating mixin forwarders before erasure is broken: - Because they're generated before erasure, their erasure in the class where their defined might differ from the erasure of the method they forward to. This is normally OK since they'll get a bridge. - However, mixin forwarders can clash with other methods as demonstrated by neg/mixin-forwarder-clash1, but these clashes won't be detected under separate compilation since double-defs checks are only done based on the signature of methods where they're defined, so neg/mixin-forwarder-clash2 fails. Checking for clashes against the signatures of all possible forwarders would be really annoying and possibly expensive, so instead the next commit solves this issue by moving mixin forwarder generation after erasure.
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Mar 18, 2019
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.
smarter
force-pushed
the
mixin-after-erasure-3
branch
from
6eefc75 to
077bb1e
Compare
* Pros
- When forwarding before erasure, we might end up needing a bridge,
the non-bridged forwarder therefore serves no practical purpose
and can cause clashes. These are problematic for two reasons:
1. Valid Scala 2 code might break, and can only be fixed in
binary incompatible way, like with
scala/scala@4366332
and
scala/scala@e3ef657
2. Clashes might not be detected under separate compilation, as
demonstrated by the previous commit.
Forwarding after erasure solves both of these problems.
- Scala 2 has always done it this way, so we're less likely to run
into surprising problems.
- Compiling the 2.12 standard library got 20% faster, I suspect that
the previous hotspot in `isCurrent` is no longer so hot when run
after erasure, so I removed the comment there.
* Cons
- When forwarding after erasure, generic signatures will be missing,
Scala 2 tries to restore this information but that doesn't always
work (scala/bug#8905). We'll either have
to do something similar, or investigate a different solution like
scala/scala#7843.
Now that mixin forwarders are generated after erasure, we may end up with a reference to ThisType(*:) after erasure in Tuple.scala, because *: extends NonEmptyTuple which erases to Product, and Product has methods that should get mixin forwarders. This fixes the tastyBootstrap test broken after the last commit.
There is only one of those so far: NonEmptyTuple which erases to Product. Without this commit, the mixin forwarders to Product in the *: class in Tuple.scala were only generated if we happened to have seen augmented Product in a previous compilation unit in the same run.
smarter
force-pushed
the
mixin-after-erasure-3
branch
from
077bb1e to
96cc754
Compare
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Mar 18, 2019
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.
smarter
added a commit
to smarter/scala
that referenced
this pull request
Mar 20, 2019
Since scala/scala3#6079, Dotty generates mixin forwarders after erasure just like Scala 2, so they don't cause extra name clashes, therefore we can revert 4366332 and e3ef657.
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.
See each commit message for more information.