Skip to content
This repository has been archived by the owner. It is now read-only.

8256641: CDS VM operations do not lock the heap #8

Conversation

@tschatzl
Copy link

@tschatzl tschatzl commented Dec 11, 2020

(Originally started in openjdk/jdk PR #1161, but the fork happened before pushing)

Hi all,

can I get reviews for this change that adds missing synchronization of CDS related VM operations with other heap operations?

VM_PopulateDumpSharedSpace, VM_PopulateDynamicDumpSharedSpace and VM_Verify are used during CDS operation, one for creating the CDS archive (eventually doing a GC), one for mapping in the CDS archive into the heap, and the last one for verification.

(Fwiw, imho the first two are awfully close and should be renamed to be better distinguishable, but that's another matter)

They all in one way or the other need to synchronize with garbage collection as they may either do a GC or just do verification, as actual (STW-)gc returns an uninitialized block of memory that is not parseable; and before that block of memory can be initialized, another VM operation like one of the mentioned could be started otherwise seeing that uninitialized memory and crashing.

The existing mechanism to prevent this kind of interference is taking the Heap_lock, so the suggested solution is based on having all these VM operations descend from a new VM_GC_Sync_Operation VM_Operation which does that (and only that), split out from VM_GC_Operation.

There some points I would like to bring up in advance in this change that may be contentious:

each VM Operation could handle Heap_lock by itself, which I considered to be too error-prone.
the need for VM_Verify to coordinate with garbage collections is new and has been introduced with JDK-8253081 as since then a Java thread might execute it - that's why this hasn't been a problem before. That could be undone (removed), but I kind of believe that with more expected changes to the CDS mechanism in the future the additional full-heap verification after loading the archive is worth the additional effort.
One (implementation) drawback is that since ZGC also uses VM_Verify, that operation now gets the Heap_lock too, and is kind of also using some part of the "set of operations related to GC" in general but did not so before, keeping almost completely separate. Testing did not show an issue, and I tried to look at the code carefully to see whether there could be issues with no result. (I.e. I couldn't find an issue). Obviously I'd like to ask you to look over this again.
so this change adds a new VM Operation class called VM_GC_Sync_Operation that splits off the handling of Heap_lock (i.e. the actual synchronizationfromVM_GC_Operation. The reason is that I do not think the logic for the gc VM operation that prevents multiple back-to-back GC operations is a good fit for any of the VM_Populate*or evenVM_Verify` operations.

Testing: tier1-5; test case attached to the CR; other known reproducers (runtime/valhalla/inlinetypes/InlineOops.java in the Valhalla repo)


Progress

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

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk16 pull/8/head:pull/8
$ git checkout pull/8

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 11, 2020

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

@tschatzl
Copy link
Author

@tschatzl tschatzl commented Dec 11, 2020

/label add hotspot-gc
/label add hotspot-runtime
/label remove hotspot

@openjdk openjdk bot added the hotspot-gc label Dec 11, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@tschatzl
The hotspot-gc label was successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@tschatzl
The hotspot-runtime label was successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@tschatzl The hotspot label was not set.

@tschatzl tschatzl marked this pull request as ready for review Dec 11, 2020
@openjdk openjdk bot added the rfr label Dec 11, 2020
@tschatzl
Copy link
Author

@tschatzl tschatzl commented Dec 11, 2020

No changes from original, applies cleany.

Copy link

@kimbarrett kimbarrett left a comment

Still looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

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

8256641: CDS VM operations do not lock the heap

Reviewed-by: kbarrett, iklam

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 Dec 11, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 11, 2020

Webrevs

iklam
iklam approved these changes Dec 11, 2020
@tschatzl
Copy link
Author

@tschatzl tschatzl commented Dec 11, 2020

Thanks @kimbarrett @iklam for your reviews.

/integrate

@openjdk openjdk bot closed this Dec 11, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 11, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@tschatzl Pushed as commit bacf22b.

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

@tschatzl tschatzl deleted the 8256641-missing-verification-heap-locking branch Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
3 participants