8265796: vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java fails when running with JEP 416

[lmesnik]

The nsk.share.jdi.TestClass1 is used via reflection. The reflective call creates MethodHandle and one more reference to TestClass1. So the number of expected references should be incremented. Thanks to @plummercj and @mlchung for the investigation.
This fix also prints references to inspected class.

---

Progress

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

Issue

• JDK-8265796: vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java fails when running with JEP 416

Reviewers

• Chris Plummer (@plummercj - Reviewer) ⚠️ Review applies to 04676de
• Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to 04676de

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6402

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

Using diff file

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

[bridgekeeper]

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

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

• serviceability

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.

[mlbridge]

Webrevs

• 03: Full (7b4ac34c)
• 02: Full - Incremental (04676dec)
• 01: Full - Incremental (196534ed)
• 00: Full (f4865603)

[openjdk]

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

8265796: vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java fails when running with JEP 416

Reviewed-by: cjplummer, 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 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.

[dholmes-ora]

Hi Leonid,

Where is the reflective call that you are referring to? I'm wondering why the MH based reflection holds a reference where the previous reflection implementation would not? And whether this is an unintended change in behaviour for core reflection.

Thanks,
David

[mlchung]

@lmesnik I also have the same question as David asks. If the reference is held via the MethodType, is TestClass1 part of the signature of the reflective call?

[lmesnik]

The reference is held via MethodType. Debugee loads TestedClass1 using reflection.

[mlchung]

I think the test calls Class::newInstance to create an instance of TestClass1.

The old implementation has one reference from Constructor::clazz field to TestClass1. The new implementation should have 2 new references: MethodHandle::member whose MemberName::clazz field references it and MethodHandle::type as the signature is ()TestClass1. I would expect 2 additional references but not 1.

@lmesnik does the debugger show MemberName::clazz?

[lmesnik]

The debugger doesn't show MemberName::clazz, but I could see it in heapdump. It seems because it doesn't have GC root and should be here by JDI spec.
And the MethodType is referred from 'static final ConcurrentWeakInternSet internTable = new ConcurrentWeakInternSet<>();’' from java/lang/invoke/MethodType.java.
See ref tree:
instance of java.lang.invoke.MethodType(id=883)
instance of java.lang.invoke.MethodType$ConcurrentWeakInternSet$WeakEntry(id=887)
instance of java.util.concurrent.ConcurrentHashMap$Node(id=888)
instance of java.util.concurrent.ConcurrentHashMap$Node[1024] (id=890)
instance of java.util.concurrent.ConcurrentHashMap(id=892)
instance of java.lang.invoke.MethodType$ConcurrentWeakInternSet(id=894)
instance of java.lang.Class(reflected class=java.lang.invoke.MethodType, id=72)

[mlchung]

I believe the Constructor object is already GC'ed. This weak entry should be GC'ed in the subsequent GC cycles and only when internTable is accessed (get or add). I thought about calling System::gc in the debuggee but it may not help because the purging won't happen until internTable is accessed. Maybe worth doing an experiment.

[lmesnik]

The triggering GC is enough to clean references. I've updated the test to force GC so we don't have this reference when checking referringObjects(long).

[mlchung]

Looks okay to me. Chris should re-review as I'm not close to the jdi tests.

[plummercj]

Looks good.

[openjdk]

@lmesnik this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8265796
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[lmesnik]

/integrate

[openjdk]

Going to push as commit 7cb56a2.

[openjdk]

@lmesnik Pushed as commit 7cb56a2.

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