8259067: bootclasspath append takes out object lock #1935

coleenp · 2021-01-04T17:28:30Z

See CR for details.
I made the classpath append list lock-free. Calling experts in Atomic operations...
Tested with tier1-6.
Thanks,
Coleen

Progress

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

Issue

JDK-8259067: bootclasspath append takes out object lock

Reviewers

Lois Foltan (@lfoltan - Reviewer) ⚠️ Review applies to 9b25060
Serguei Spitsyn (@sspitsyn - Reviewer) ⚠️ Review applies to 9b25060
Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to 9b25060
David Holmes (@dholmes-ora - Reviewer)

Download

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

bridgekeeper · 2021-01-04T17:28:44Z

👋 Welcome back coleenp! 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.

openjdk · 2021-01-04T17:29:57Z

@coleenp The following labels will be automatically applied to this pull request:

hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-01-04T17:43:18Z

Webrevs

src/hotspot/share/classfile/classLoader.cpp

coleenp · 2021-01-04T18:50:36Z

For the record, these tests exercise this code:
vmTestbase/nsk/jvmti/AddToBootstrapClassLoaderSearch/bootclssearch007,8,9,10.

lfoltan

LGTM.
Lois

openjdk · 2021-01-04T19:23:49Z

@coleenp This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8259067: bootclasspath append takes out object lock

Reviewed-by: lfoltan, sspitsyn, iklam, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 66 new commits pushed to the master branch:

67c2211: 8259045: Exception message from saproc.dll is garbled on Windows with Japanese locale
28e1f4d: 8247957: remove doclint support for HTML 4
80544e4: 8250564: Remove terminally deprecated constructor in GSSUtil
940b053: 8259232: Bad JNI lookup during printing
df721f0: 8259291: Cleanup unnecessary local variables
d20d2fa: 8258143: Update --release 16 symbol information for JDK 16 build 30 or later
3be6e06: 8259312: VerifyCACerts.java fails as soneraclass2ca cert will expire in 90 days
722f236: 8259231: Epsilon: improve performance under contention during virtual space expansion
f6cb8c5: 8258908: Remove JVM option CleanChunkPoolAsync
c0540ff: 8231627: ThreadsListHandleInErrorHandlingTest.java fails in printing all threads
... and 56 more: https://git.openjdk.java.net/jdk/compare/244573509deef62bcf0abd18d4ccc963596b65ec...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

coleenp · 2021-01-04T21:37:34Z

Thanks, Lois!

sspitsyn

Hi Coleen,

LGTM++

Thanks,
Serguei

coleenp · 2021-01-05T04:19:29Z

Thanks Serguei. Ioi convinced me offline to simplify and just use a mutex, so I updated the patch to do that. I tested with jdi and jvmti tests and will rerun tier1-3.

sspitsyn · 2021-01-05T21:10:45Z

Coleen, the function ClassLoader::add_to_boot_append_entries is also called from ClassLoader::update_class_path_entry_list and ClassLoaderExt::append_boot_classpath. It is not clear why the Bootclasspath_lock is not used for sync in these contexts. Could you consider to move this MutexLocker from JvmtiEnv::AddToBootstrapClassLoaderSearch to ClassLoader::add_to_boot_append_entries?

coleenp · 2021-01-05T22:26:11Z

Thanks Serguei. I was trying to decide whether to move the lock there or add an assert that we are not multi-threaded when the lock isn't held. But it's cleaner to just move the lock there, so I'll do that.

coleenp · 2021-01-06T00:14:40Z

I reran tier1 and jvmti and jdi tests.

iklam

LGTM

sspitsyn

LGTM++

lfoltan

Changes look good to me.
Lois

dholmes-ora

Hi Coleen,

Just a quick skim, but I think you need acquire/release for the lock-free path.

Thanks,
David

src/hotspot/share/classfile/classLoader.cpp

src/hotspot/share/classfile/classLoader.hpp

dholmes-ora

Addition of release/acquire looks good. Note that we now update _first_append_entry with the same release semantics as ClassPathEntry::set_next - as we should in general.

I'm not 100% clear on which part of these API's can only be called whilst the VM is still single-threaded, but ignoring that, the change to use locked updates with lock-free traversal seems functionally correct. The overall necessity of using locking does depend on the potential for concurrent calls to ClassLoader::add_to_boot_append_entries - something I have not investigated at all.

Thanks,
David

coleenp · 2021-01-07T03:09:20Z

Thanks for reviewing this, David. The concurrent calls are from JVMTI where the lock was originally, but as suggested by Serguei, for safety, and to avoid tracing through all the callers, I moved the locking to the function where we write the list. It's not a performance critical operation.

coleenp · 2021-01-07T12:34:36Z

/integrate

openjdk · 2021-01-07T12:35:41Z

@coleenp Since your change was applied there have been 69 commits pushed to the master branch:

0e6de4e: 8259339: AllocateUninitializedArray C2 intrinsic fails with void.class input
81c0624: 8259354: Fix race condition in AbstractEventStream.nextThreadName
227f99d: 8233555: [TESTBUG] JRadioButton tests failing on MacoS
67c2211: 8259045: Exception message from saproc.dll is garbled on Windows with Japanese locale
28e1f4d: 8247957: remove doclint support for HTML 4
80544e4: 8250564: Remove terminally deprecated constructor in GSSUtil
940b053: 8259232: Bad JNI lookup during printing
df721f0: 8259291: Cleanup unnecessary local variables
d20d2fa: 8258143: Update --release 16 symbol information for JDK 16 build 30 or later
3be6e06: 8259312: VerifyCACerts.java fails as soneraclass2ca cert will expire in 90 days
... and 59 more: https://git.openjdk.java.net/jdk/compare/244573509deef62bcf0abd18d4ccc963596b65ec...master

Your commit was automatically rebased without conflicts.

Pushed as commit 1c33847.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8259067: bootclasspath append takes out object lock

f132d7b

openjdk bot added the rfr Pull request is ready for review label Jan 4, 2021

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jan 4, 2021

zhengyu123 reviewed Jan 4, 2021

View reviewed changes

src/hotspot/share/classfile/classLoader.cpp Outdated Show resolved Hide resolved

lfoltan approved these changes Jan 4, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 4, 2021

sspitsyn approved these changes Jan 5, 2021

View reviewed changes

Simplify with a Mutex.

fb8e87d

Move locking into function where exclusive access is needed.

9b25060

iklam approved these changes Jan 6, 2021

View reviewed changes

sspitsyn approved these changes Jan 6, 2021

View reviewed changes

lfoltan approved these changes Jan 6, 2021

View reviewed changes

dholmes-ora suggested changes Jan 6, 2021

View reviewed changes

src/hotspot/share/classfile/classLoader.cpp Outdated Show resolved Hide resolved

src/hotspot/share/classfile/classLoader.hpp Outdated Show resolved Hide resolved

Use acquire/release for first_append_entry_list

1e7251b

dholmes-ora approved these changes Jan 7, 2021

View reviewed changes

openjdk bot closed this Jan 7, 2021

openjdk bot added the integrated Pull request has been integrated label Jan 7, 2021

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 7, 2021

coleenp deleted the classpath-append-lock branch January 7, 2021 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8259067: bootclasspath append takes out object lock #1935

8259067: bootclasspath append takes out object lock #1935

coleenp commented Jan 4, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 4, 2021

openjdk bot commented Jan 4, 2021

mlbridge bot commented Jan 4, 2021 •

edited

Loading

coleenp commented Jan 4, 2021

lfoltan left a comment

openjdk bot commented Jan 4, 2021 •

edited

Loading

coleenp commented Jan 4, 2021

sspitsyn left a comment

coleenp commented Jan 5, 2021

sspitsyn commented Jan 5, 2021

coleenp commented Jan 5, 2021

coleenp commented Jan 6, 2021

iklam left a comment

sspitsyn left a comment

lfoltan left a comment

dholmes-ora left a comment

dholmes-ora left a comment

coleenp commented Jan 7, 2021

coleenp commented Jan 7, 2021

openjdk bot commented Jan 7, 2021

8259067: bootclasspath append takes out object lock #1935

8259067: bootclasspath append takes out object lock #1935

Conversation

coleenp commented Jan 4, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Jan 4, 2021

openjdk bot commented Jan 4, 2021

mlbridge bot commented Jan 4, 2021 • edited Loading

Webrevs

coleenp commented Jan 4, 2021

lfoltan left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 4, 2021 • edited Loading

coleenp commented Jan 4, 2021

sspitsyn left a comment

Choose a reason for hiding this comment

coleenp commented Jan 5, 2021

sspitsyn commented Jan 5, 2021

coleenp commented Jan 5, 2021

coleenp commented Jan 6, 2021

iklam left a comment

Choose a reason for hiding this comment

sspitsyn left a comment

Choose a reason for hiding this comment

lfoltan left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

coleenp commented Jan 7, 2021

coleenp commented Jan 7, 2021

openjdk bot commented Jan 7, 2021

coleenp commented Jan 4, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 4, 2021 •

edited

Loading

openjdk bot commented Jan 4, 2021 •

edited

Loading