SD-233 synchronized blocks are JIT-friendly again#5417
Conversation
41cc60f to
d9852c1
Compare
|
Review by @lrytz |
|
LGTM Another lesson in the arts of taming the mighty JIT gods.. |
|
@retronym here's a regression test: https://github.com/lrytz/scala/tree/sd233-test I tried pushing to your branch, but Is "Allow edits from maintainers" checked? https://github.com/blog/2247-improving-collaboration-with-forks |
d9852c1 to
dc6f9a9
Compare
|
@lrytz I haven't seen that option when pushing to scala/scala. I did see it pushing to sbt/sbt. Not sure why. I've pushed your commit with the test, thanks! I'd like to merge this soon and then restarr the 2.12.0 branch. I can then rebase my other performance oriented PRs and benchmark them (along with the residual commits in https://github.com/scala/scala/compare/2.12.x...retronym:opt/settings?expand=1) to see which ones still are worthwhile. |
|
I don't see it when creating a PR, but in my PR #5415 it's there on the right
|
|
Clicked! |
|
OK, so it was off - for some reason it was on in my PR by default.. |
|
I created the PR with the command line tool, |
|
Wow! Could you add a comment to the code as well? Also, I would be curious to see if this removes the need for the compute method for modules. |
GenBCode, the new backend in Scala 2.12, subtly changed the way that synchronized blocks are emitted. It used `java/lang/Throwable` as an explicitly named exception type, rather than implying the same by omitting this in bytecode. This appears to confuse HotSpot JIT, which reports a error parsing the bytecode into its IR which leaves the enclosing method stuck in interpreted mode. This commit passes a `null` descriptor to restore the old pattern (the same one used by javac.) I've checked that the JIT warnings are gone and that the method can be compiled again.
dc6f9a9 to
e994c1c
Compare
|
@adriaanm Comment added. I'll benchmark the compiler bootstrapped through this change and reversion of the module lzycompute method, but I'd like to do that as a followup task. |
|
Thanks, sounds good! |
|
Restarring proposed: #5422 |
GenBCode, the new backend in Scala 2.12, subtly changed
the way that synchronized blocks are emitted.
It used
java/lang/Throwableas an explicitly named exceptiontype, rather than implying the same by omitting this in bytecode.
This appears to confuse HotSpot JIT, which reports a error
parsing the bytecode into its IR which leaves the enclosing method
stuck in interpreted mode.
This commit passes a
nulldescriptor to restore the old pattern(the same one used by javac.) I've checked that the JIT warnings
are gone and that the method can be compiled again.
Fixes scala/scala-dev#233