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

8273000: Remove WeakReference-based class initialisation barrier implementation #5258

Closed
wants to merge 3 commits into from

Conversation

iwanowww
Copy link

@iwanowww iwanowww commented Aug 25, 2021

Get rid of WeakReference-based logic in DirectMethodHandle::checkInitialized() and reimplement it with Unsafe::ensureClassInitialized()/shouldBeInitialized().

The key observation is that Unsafe::ensureClassInitialized() does not block the initializing thread.

Also, removed Unsafe::shouldBeInitialized() in DMH::shouldBeInitialized(MemberName) to save on calling into the VM.
Unsafe::ensureClassInitialized() already has a fast-path check which checks whether the class is fully initialized or not.

Testing: tier1 - tier6


Progress

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

Issue

  • JDK-8273000: Remove WeakReference-based class initialisation barrier implementation

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5258/head:pull/5258
$ git checkout pull/5258

Update a local copy of the PR:
$ git checkout pull/5258
$ git pull https://git.openjdk.java.net/jdk pull/5258/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5258

View PR using the GUI difftool:
$ git pr show -t 5258

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5258.diff

@iwanowww iwanowww changed the title Get rid of WeakReference-based logic in DirectMethodHandle::checkInitialized() and reimplement it with Unsafe::ensureClassInitialized()/shouldBeInitialized(). 8273000: Remove WeakReference-based class initialisation barrier implementation Aug 25, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 25, 2021

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

@iwanowww iwanowww marked this pull request as ready for review Aug 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 25, 2021

@iwanowww The following label will be automatically applied to this pull request:

  • core-libs

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

@openjdk openjdk bot added core-libs rfr labels Aug 25, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 25, 2021

Webrevs

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Aug 26, 2021

The key observation is that Unsafe::ensureClassInitialized() does not block the initializing thread.

I'm unclear exactly what that statement is meant to indicate. The thread actually running "clinit" does not block due to the initialization process, but will block if the class initialization code blocks. If the class is not initialized and you are racing with another thread doing the initialization then you may block until that initialization is complete.

@iwanowww
Copy link
Author

@iwanowww iwanowww commented Aug 26, 2021

I'm unclear exactly what that statement is meant to indicate.

DirectMethodHandle::checkInitialized() is dual-purpose: it implements class initialization barrier and reports whether the class is fully initialized, so the barrier can be safely elided.

The call to Unsafe::ensureClassInitialized() blocks until initialization is over when the current thread is not the initializing one.

But when call to Unsafe::ensureClassInitialized() finished, there are 2 cases possible:

  • the class is fully initialized;
  • the class is being initialized and the current thread is the initializing one.

In the former case, it's safe to remove the barrier while in the latter the barrier is still required.
Original implementation implemented that in an explicit manner by using WeakReferences to record the initializing thread.
But a pair of Unsafe::ensureClassInitialized() and Unsafe::shouldBeInitialized() calls provides equivalent functionality and is much simpler at the same time.

Copy link
Member

@PaulSandoz PaulSandoz left a comment

That's a nice cleanup to a tricky area (one of the few used to trigger an update a final field). In effect we were already relying on that behavior in the ClassValue computation.

May i suggest that we add some JavaDoc to ensureClassInitialized describing the two cases of the calling thread is the initialization thread or not.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 26, 2021

@iwanowww 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:

8273000: Remove WeakReference-based class initialisation barrier implementation

Reviewed-by: psandoz, mchung

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 96 new commits pushed to the master branch:

  • c2e015c: 8273229: Update OS detection code to recognize Windows Server 2022
  • 0c1b16b: 8273243: Fix indentations in java.net.InetAddress methods
  • 152e669: 8273140: Replace usages of Enum.class.getEnumConstants() with Enum.values() where possible
  • 857a930: 8263375: Support stack watermarks in Zero VM
  • 6cfe314: 8272970: Parallelize runtime/InvocationTests/
  • a9a83b2: 8273256: runtime/cds/appcds/TestEpsilonGCWithCDS.java fails due to Unrecognized VM option 'ObjectAlignmentInBytes=64' on x86_32
  • 1a5a2b6: 8271745: Correct block size for KW,KWP mode and use fixed IV for KWP mode for SunJCE
  • 2f01a6f: 8273157: Add convenience methods to Messager
  • 9689f61: 8272788: Nonleaf ranked locks should not be safepoint_check_never
  • 4ee0dac: 8273248: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh on all configs
  • ... and 86 more: https://git.openjdk.java.net/jdk/compare/7f80683cfeee3c069f48d5bce45fa92b2381b518...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.

@openjdk openjdk bot added the ready label Aug 26, 2021
UNSAFE.ensureClassInitialized(defc); // initialization barrier; blocks unless called by the initializing thread
return !UNSAFE.shouldBeInitialized(defc); // keep the barrier if the initialization is still in progress
Copy link
Member

@dholmes-ora dholmes-ora Aug 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some more elaborate commentary about the possibility of this being called while <clinit> of defc is already on the call stack, would be worthwhile - the existing comments are a little too subtle IMO.

UNSAFE.ensureClassInitialized(defc);
// Once we get here either defc was fully initialized by another thread, or
// defc was already being initialized by the current thread. In the latter case
// the barrier must remain. We can detect this simply by checking if initialization
// is still needed.
return !UNSAFE.shouldBeInitialized(defc);

Copy link
Author

@iwanowww iwanowww Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, David. I incorporated your suggestion in the latest version.

@mlchung
Copy link
Member

@mlchung mlchung commented Aug 31, 2021

May i suggest that we add some JavaDoc to ensureClassInitialized describing the two cases of the calling thread is the initialization thread or not.

This is a good suggestion that also applies to Lookup::ensureInitialized. I created https://bugs.openjdk.java.net/browse/JDK-8273194 to improve the javadoc for Lookup::ensureInitialized.

@iwanowww
Copy link
Author

@iwanowww iwanowww commented Sep 1, 2021

May i suggest that we add some JavaDoc to ensureClassInitialized

Thanks, Paul. How does the latest version look?

Copy link
Member

@PaulSandoz PaulSandoz left a comment

Looks good, just a minor suggestion up to you to accept or not.

* The call returns when either class {@code c} is fully initialized or
* class {@code c} is being initialized and the call is performed from
* the initializing thread.
Copy link
Member

@PaulSandoz PaulSandoz Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* The call returns when either class {@code c} is fully initialized or
* class {@code c} is being initialized and the call is performed from
* the initializing thread.
* The call returns when either class {@code c} is fully initialized or
* class {@code c} is being initialized and the call is performed from
* the initializing thread. In the latter case a subsequent call to
* {@link #shouldBeInitialized}, from the calling thread of this call,
* will return {@code true}.

Copy link
Member

@dholmes-ora dholmes-ora Sep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't "the calling thread of this call" and "the initializing thread" the same thread in the latter case?

Copy link
Author

@iwanowww iwanowww Sep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I dropped "from the calling thread of this call" part and incorporated the rest into the latest version.

@iwanowww
Copy link
Author

@iwanowww iwanowww commented Sep 8, 2021

Thanks for the reviews, Mandy, Paul, and David.

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 8, 2021

Going to push as commit faa942c.
Since your change was applied there have been 151 commits pushed to the master branch:

  • 21012f2: 8078641: MethodHandle.asTypeCache can retain classes from unloading
  • 1855574: 8273038: ChannelInputStream.transferTo() uses FileChannel.transferTo(FileChannel)
  • 6750c34: 8270533: AArch64: size_fits_all_mem_uses should return false if its output is a CAS
  • a66629a: 8254167: G1: Record regions where evacuation failed to provide targeted iteration
  • 286a1f6: 8273440: Zero: Disable runtime/Unsafe/InternalErrorTest.java
  • 7d24a33: 8273318: Some containers/docker/TestJFREvents.java configs are running out of memory
  • 1513dc7: 8271603: Unnecessary Vector usage in java.desktop
  • ea4907a: 8273047: test jfr/api/consumer/TestRecordedFrame.java timing out
  • 4eacdb3: 8273104: Refactoring option parser for UL
  • 8884d2f: 8273462: ProblemList vmTestbase/vm/mlvm/anonloader/stress/oome/heap/Test.java in -Xcomp mode
  • ... and 141 more: https://git.openjdk.java.net/jdk/compare/7f80683cfeee3c069f48d5bce45fa92b2381b518...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 8, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 8, 2021

@iwanowww Pushed as commit faa942c.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
4 participants