8214972: Uses of klass_holder() except GC need to apply GC barriers

[fandreuz]

I've been working on a backport to fix JDK-8339725. The fix attached to the ticket is not enough to fix the problem on jdk11.

The missing backport is JDK-8214972, which I apply in this PR. JDK-8210321 is also needed to provide ClassLoaderData::holder_no_keepalive.

Tier1 tests completed successfully.

---

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
• JDK-8210321 needs maintainer approval
• JDK-8214972 needs maintainer approval

Issues

• JDK-8214972: Uses of klass_holder() except GC need to apply GC barriers (Bug - P3 - Rejected)
• JDK-8210321: Create NO_KEEPALIVE CLD holder accessor (Enhancement - P4 - Rejected)

Reviewers

• Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/3059/head:pull/3059
$ git checkout pull/3059

Update a local copy of the PR:
$ git checkout pull/3059
$ git pull https://git.openjdk.org/jdk11u-dev.git pull/3059/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3059

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/3059.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back fandreuz! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

This backport pull request has now been updated with issue and summary from the original commit.

[fandreuz]

/issue add JDK-8210321

[openjdk]

@fandreuz
Adding additional issue to issue list: 8210321: Create NO_KEEPALIVE CLD holder accessor.

[mlbridge]

Webrevs

• 00: Full (d7b59984)

[phohensee]

This looks ok, but why is it needed as a prereq for JDK-8339725?

Given that 11u is old, why do you want to backport it? Do you have production use issues? I recommend running both tier1 and tier2, with both fastdebug and release JDKs.

[fandreuz]

@phohensee

This looks ok, but why is it needed as a prereq for JDK-8339725?

It's difficult to pinpoint the specific issue as the problem manifests as a segfault in an unrelated location. I determined JDK-8214972 is needed by bisecting the history and cherry-picking JDK-8339725 into each commit. The commit related to JDK-8214972 is the first where the reproducer attached to JDK-8339725 runs successfully. Applying both JDK-8339725 and JDK-8214972 to jdk11 solves the problem indeed.

Given that 11u is old, why do you want to backport it? Do you have production use issues?

We had an occurrence of this issue in async-profiler/async-profiler#974. As reported by the user, the problem affects jdk11 too. JDK-8339725 was already backported to jdk17: openjdk/jdk17u-dev#3646.

I recommend running both tier1 and tier2, with both fastdebug and release JDKs.

I ran tier1 with release already, I'll start a tier2 run with fastdebug.

[fandreuz]

@phohensee I ran the following test suites:

• release
  • tier1: all tests pass
  • tier2: all tests pass except for:
    • ConnectionReuseTest::testConnReuse
    • DirectIOTest
• fastdebug
  • tier1: all tests pass
  • tier2: all tests pass except for:
    • ConnectionReuseTest::testConnReuse
    • DirectIOTest

The failures don't seem to be related to my changes, possibly flaky tests? I found tickets related to them (e.g. JDK-8305900) which haven't been backported to jdk11.

[phohensee]

Thanks for the explanation and additional testing.

[openjdk]

⚠️ @fandreuz This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[openjdk]

@fandreuz
8214972: The approval request has been created successfully.
8210321: The approval request has been created successfully.

[fandreuz]

/approval request Backporting as a prerequisite to backport and fix JDK-8339725. We had an occurrence of that crash, reported by an Async-Profiler user: async-profiler/async-profiler#974

The two backports in this PR apply almost cleanly, except for some trivial conflicts due to the fact that JDK-8209301 has not been backported to jdk11.

Ran tier1 and tier2 test suites with release and fastdebug, I didn't notice any regression.

[openjdk]

@fandreuz
8214972: The approval request has been updated successfully.
8210321: The approval request has been updated successfully.

[jerboaa]

/approve no

The risk of introducing this change doesn't outweigh the benefit of fixing an async profiler issue at this point in time of JDK 11 life-cycle. Users need to either a) run without async-profiler b) upgrade to JDK 17.

[openjdk]

@jerboaa
8214972: The approval request has been rejected.
8210321: The approval request has been rejected.