Skip to content
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

8258015: [JVMCI] JVMCI_lock shouldn't be held while initializing box classes #1727

Closed
wants to merge 1 commit into from

Conversation

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Dec 10, 2020

Hi all,

Plenty of AOT tests failed after JDK-8257917.
The reason is that JVMCI_lock [1] was held during box classes initilization.
However, this assert [2] doesn't allow a lock (except tty_lock [3]) to be held in that case.
So JVMCI_lock here [1] should be removed.

Testing:
compiler/aot and compiler/jvmci on Linux/x64

Thanks.
Best regards,
Jie

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/jvmci/jvmci.cpp#L133
[2] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/thread.cpp#L795
[3] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/mutex.cpp#L440


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8258015: [JVMCI] JVMCI_lock shouldn't be held while initializing box classes

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1727/head:pull/1727
$ git checkout pull/1727

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 10, 2020

👋 Welcome back jiefu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Dec 10, 2020

/issue add JDK-8258015
/test
/label add hotspot-compiler
/cc hotspot-compiler

@openjdk openjdk bot added the rfr label Dec 10, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 10, 2020

@DamonFool This issue is referenced in the PR title - it will now be updated.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 10, 2020

@DamonFool
The hotspot-compiler label was successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 10, 2020

@DamonFool The hotspot-compiler label was already applied.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 10, 2020

Webrevs

@veresov
Copy link
Contributor

@veresov veresov commented Dec 10, 2020

Yes, seems like, we can safely get rid of this lock since the init state change in InstanceKlass is already under a lock. Worst case scenario we do that loop a few times. @dougxc, what do you think?

Alternatively, we can drop the checks of the initialized flag for AOT entirely.

@@ -130,11 +130,6 @@ void JVMCI::ensure_box_caches_initialized(TRAPS) {
if (_box_caches_initialized) {
return;
}
MutexLocker locker(JVMCI_lock);
Copy link
Member

@dougxc dougxc Dec 10, 2020

This lock is required in the case where this is not called from AOTLoader::initialize_box_caches to ensure only one JVMCI compiler thread does the lazy initialization.
I can fix this if you'd like me to put up a separate PR.

Copy link
Member Author

@DamonFool DamonFool Dec 10, 2020

OK. Please feel free to do so.
Thanks.

Copy link
Member

@dougxc dougxc Dec 10, 2020

I've opened #1730.

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