8280454: G1: ClassLoaderData verification keeps CLDs live that causes problems with VerifyDuringGC during Remark #8949

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.

Sign up for GitHub

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

Jump to bottom

Closed

tschatzl wants to merge 5 commits into openjdk:master from tschatzl:submit/8280454-cldg-iteration-verifyduringgc

Contributor

tschatzl commented May 30, 2022 •

edited by openjdk bot

Loading

Hi all,

can I have reviews for this change that makes a few CLDG iterations no-keepalive during GC pauses?

This fixes a bug where the VerifyDuringGC verification otherwise kept alive all classes, effectively disabling class unloading during (g1) concurrent mark.

Afaict this verification code is only every called during GC pauses, so the change should be fine.

As for the change itself, I tried to avoid cluttering the code with ClassLoaderDataGraphIterator<> by using typedefs - unfortunately only C++ 17 allows omitting the <> if all template parameters are defaulted.

There is also a new test case for just this.

Testing: tier1-5

Thanks,
Thomas

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8280454: G1: ClassLoaderData verification keeps CLDs live that causes problems with VerifyDuringGC during Remark

Reviewers

Stefan Karlsson (@stefank - Reviewer)
Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/8949/head:pull/8949
$ git checkout pull/8949

Update a local copy of the PR:
$ git checkout pull/8949
$ git pull https://git.openjdk.org/jdk pull/8949/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8949

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/8949.diff


          8280454: G1: ClassLoaderData verification keeps CLDs live that causes…

f7b70b3

… problems with VerifyDuringGC during Remark

Contributor Author

tschatzl commented May 30, 2022

/label add hotspot-runtime

bridgekeeper bot commented May 30, 2022

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

openjdk bot added the hotspot-runtime label

openjdk bot commented May 30, 2022

@tschatzl
The hotspot-runtime label was successfully added.

tschatzl marked this pull request as ready for review

May 31, 2022 07:43

openjdk bot added the rfr label

mlbridge bot commented May 31, 2022 •

edited

Loading

Webrevs

stefank reviewed

View reviewed changes

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

stefank suggested changes

View reviewed changes

Member

stefank left a comment

It's unclear to me why this works. You've removed the "keep alive" property from the iterator, but it looks like the verification code still uses "keep alive" loads on the class loaders.

Could you explain how this patch prevents those class loaders from being kept alive?

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

src/hotspot/share/classfile/classLoaderDataGraph.cpp

    
                ClassLoaderDataGraphIteratorNoKeepAlive iter;

                while (ClassLoaderData* cld = iter.get_next()) {

                  if (cld->dictionary() != nullptr) {

                    cld->dictionary()->verify();

Member

stefank Jun 1, 2022

If I'm reading the code correctly, it seems like this function keeps the class loader alive:

void Dictionary::verify() {
  guarantee(number_of_entries() >= 0, "Verify of dictionary failed");

  ClassLoaderData* cld = loader_data();
  // class loader must be present;  a null class loader is the
  // bootstrap loader
  guarantee(cld != NULL &&
            (cld->the_null_class_loader_data() || cld->class_loader()->is_instance()),
            "checking type of class_loader");

The cld->class_loader() call resolves the OopHandle, which should keep it alive.

Contributor Author

tschatzl Jun 1, 2022

That never happens because afaict cld->the_null_class_loader_data() is always non-NULL except during very early bootstrapping. I do not think it can ever be NULL for a java instantiated class loader, so the second clause is never executed there. Some additional logging in the test proved that.

I can find three where this idiom is used to see whether the null CLD is initialized - I could add a static is_null_class_loader_data_initialized() predicate to ClassLoaderData - preferably in a separate CR.

Contributor

coleenp Jun 1, 2022

Yes, in this CR you should change "the_null_class_loader_data()" to "is_the_null_class_loader_data()" and verify that your test still works.

src/hotspot/share/classfile/classLoaderDataGraph.cpp Show resolved Hide resolved

tschatzl added 2 commits

June 1, 2022 12:22


          stefank review

9e66fc7


          Remove unnecessary test change

7f16efe

coleenp approved these changes

View reviewed changes

Contributor

coleenp left a comment

This seems okay and worth adding the ClassLoaderdataGraphIteratorBase for this case. Please make the change in my comment though with this change, and retest your test.

src/hotspot/share/classfile/classLoaderDataGraph.cpp

    
                ClassLoaderDataGraphIteratorNoKeepAlive iter;

                while (ClassLoaderData* cld = iter.get_next()) {

                  if (cld->dictionary() != nullptr) {

                    cld->dictionary()->verify();

Contributor

coleenp Jun 1, 2022

Yes, in this CR you should change "the_null_class_loader_data()" to "is_the_null_class_loader_data()" and verify that your test still works.

openjdk bot commented Jun 1, 2022 •

edited

Loading

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

8280454: G1: ClassLoaderData verification keeps CLDs live that causes problems with VerifyDuringGC during Remark

Reviewed-by: stefank, coleenp

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

900d967: 8287924: Avoid redundant HashMap.containsKey call in EnvHelp.mapToHashtable
d482d7f: 8286160: (fs) Files.exists returns unexpected results with C:\pagefile.sys because it's not readable
edff51e: 8284858: Start of release updates for JDK 20
2671443: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100
59b0de6: 8288048: Build failure with GCC 6 after JDK-8286562
db4405d: 8288078: linux-aarch64-optimized build fails in Tier5 after JDK-8287567
7e948f7: 8287903: Reduce runtime of java.math microbenchmarks
3fa9984: 8287854: Dangling reference in ClassVerifier::verify_class
4285e3d: 8288023: AArch64: disable PAC-RET when preview is enabled
560e292: 8287738: [PPC64] jdk/incubator/vector/*VectorTests failing
... and 180 more: https://git.openjdk.org/jdk/compare/73ba7fdce838ba8a2c227a972c176311e6cc0b41...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 bot added the ready label


          coleenp, stefank reviews

59911ea

coleenp approved these changes

View reviewed changes

Contributor

coleenp left a comment

Still good.


          Change Dictionary verification to use keep_alive peeking of the _clas…

c67021e

…s_loader

coleenp approved these changes

View reviewed changes

Contributor

coleenp left a comment

Looks good. Thank you for fixing this.

stefank approved these changes

View reviewed changes

Contributor Author

tschatzl commented Jun 10, 2022

Thanks @stefank @coleenp for your reviews
/integrate

openjdk bot commented Jun 10, 2022

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

900d967: 8287924: Avoid redundant HashMap.containsKey call in EnvHelp.mapToHashtable
d482d7f: 8286160: (fs) Files.exists returns unexpected results with C:\pagefile.sys because it's not readable
edff51e: 8284858: Start of release updates for JDK 20
2671443: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100
59b0de6: 8288048: Build failure with GCC 6 after JDK-8286562
db4405d: 8288078: linux-aarch64-optimized build fails in Tier5 after JDK-8287567
7e948f7: 8287903: Reduce runtime of java.math microbenchmarks
3fa9984: 8287854: Dangling reference in ClassVerifier::verify_class
4285e3d: 8288023: AArch64: disable PAC-RET when preview is enabled
560e292: 8287738: [PPC64] jdk/incubator/vector/*VectorTests failing
... and 180 more: https://git.openjdk.org/jdk/compare/73ba7fdce838ba8a2c227a972c176311e6cc0b41...master

Your commit was automatically rebased without conflicts.

openjdk bot added the integrated label

openjdk bot closed this

openjdk bot removed ready rfr labels

openjdk bot commented Jun 10, 2022

@tschatzl Pushed as commit 94b473e.

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

tschatzl deleted the submit/8280454-cldg-iteration-verifyduringgc branch

July 8, 2022 15:11

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

Labels

hotspot-runtime integrated