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

SD-233 synchronized blocks are JIT-friendly again #5417

Merged
merged 1 commit into from Sep 27, 2016

Conversation

Projects
None yet
4 participants
@retronym
Member

retronym commented Sep 26, 2016

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.

Fixes scala/scala-dev#233

@scala-jenkins scala-jenkins added this to the 2.12.0-RC2 milestone Sep 26, 2016

@retronym retronym changed the title from synchronized blocks are JIT-friendly again to SD-233 synchronized blocks are JIT-friendly again Sep 26, 2016

@retronym

This comment has been minimized.

Member

retronym commented Sep 26, 2016

Review by @lrytz

@lrytz

This comment has been minimized.

Member

lrytz commented Sep 26, 2016

LGTM

Another lesson in the arts of taming the mighty JIT gods..

@lrytz

This comment has been minimized.

Member

lrytz commented Sep 26, 2016

@retronym here's a regression test: https://github.com/lrytz/scala/tree/sd233-test

I tried pushing to your branch, but

 ! [remote rejected] ticket/SD-233 -> ticket/SD-233 (permission denied)

Is "Allow edits from maintainers" checked? https://github.com/blog/2247-improving-collaboration-with-forks

@retronym

This comment has been minimized.

Member

retronym commented Sep 26, 2016

@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.

@lrytz

This comment has been minimized.

Member

lrytz commented Sep 26, 2016

I don't see it when creating a PR, but in my PR #5415 it's there on the right

image

@retronym

This comment has been minimized.

Member

retronym commented Sep 26, 2016

Clicked!

@lrytz

This comment has been minimized.

Member

lrytz commented Sep 26, 2016

OK, so it was off - for some reason it was on in my PR by default..

@retronym

This comment has been minimized.

Member

retronym commented Sep 26, 2016

I created the PR with the command line tool, hub. Perhaps the API defaults to the status quo, whereas the UI is different.

@adriaanm

This comment has been minimized.

Member

adriaanm commented Sep 26, 2016

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.

SD-233 synchronized blocks are JIT-friendly again
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.
@retronym

This comment has been minimized.

Member

retronym commented Sep 26, 2016

@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.

@adriaanm

This comment has been minimized.

Member

adriaanm commented Sep 26, 2016

Thanks, sounds good!

@retronym retronym merged commit ceaf419 into scala:2.12.0 Sep 27, 2016

5 checks passed

cla @retronym signed the Scala CLA. Thanks!
Details
integrate-ide [52] SUCCESS. Took 2 s.
Details
validate-main [69] SUCCESS. Took 133 min.
Details
validate-publish-core [68] SUCCESS. Took 10 min.
Details
validate-test [51] SUCCESS. Took 119 min.
Details
@retronym

This comment has been minimized.

Member

retronym commented Sep 27, 2016

Restarring proposed: #5422

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment