8256167: Convert JDK use of `Reference::get` to `Reference::refersTo` #1609

mlchung · 2020-12-03T22:54:54Z

This patch replaces some uses of Reference::get to Reference::refersTo to avoid keeping a referent strongly reachable that could cause unnecessary delay in collecting such object. I only made change in some but not all classes in core libraries when working with Kim on Reference::refersTo. The remaining uses are left for the component owners to convert at appropriate time.

Progress

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

Issue

JDK-8256167: Convert JDK use of Reference::get to Reference::refersTo

Reviewers

Serguei Spitsyn (@sspitsyn - Reviewer) ⚠️ Review applies to b1e645b
Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to b1e645b
Daniel Fuchs (@dfuch - Reviewer) ⚠️ Review applies to b1e645b
Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to b1e645b
Kim Barrett (@kimbarrett - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1609/head:pull/1609
$ git checkout pull/1609

…into refersTo

bridgekeeper · 2020-12-03T22:55:06Z

👋 Welcome back mchung! 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 · 2020-12-03T22:55:39Z

⚠️ @mlchung This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk · 2020-12-03T22:55:59Z

@mlchung The following labels will be automatically applied to this pull request:

core-libs
hotspot-compiler
serviceability

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

mlchung · 2020-12-03T22:56:55Z

/label remove hotspot-compiler

openjdk · 2020-12-03T22:57:21Z

@mlchung
The hotspot-compiler label was successfully removed.

mlbridge · 2020-12-03T23:00:03Z

Webrevs

01: Full (72947eb)
00: Full (b1e645b)

sspitsyn

Hi Mandy,
Looks good. I guess, you are going to update the copyright comments before the push.
Thanks,
Serguei

openjdk · 2020-12-04T03:08:56Z

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

8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

Reviewed-by: sspitsyn, shade, dfuchs, alanb, kbarrett

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

d3ac1bf: 8198390: Test MultiResolutionDrawImageWithTransformTest.java fails when -esa is passed
51d325e: 8257633: Missing -mmacosx-version-min=X flag when linking libjvm
e27ea4d: 8257750: writeBuffer field of java.io.DataOutputStream should be final
dd0b945: 8257531: Super word not applied to a loop of simple Buffer operations
d76039d: 8257725: No throws of SSLHandshakeException
fcc8479: 8257724: Incorrect package of the linked class in BaseSSLSocketImpl

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

shipilev

Replacements look fine to me.

shipilev · 2020-12-04T09:19:23Z

src/java.base/share/classes/java/util/WeakHashMap.java

+        // keeping a strong reference to the entry's referent
+        if (e.refersTo(key)) return true;
+
+        // then checks for equality


Obnoxiously minor nit: plurality is inconsistent. check if the given entry... above, and then check[s] for equality. Should be ...then check for equality?

OK. Fixed this grammatical nit.

dfuch

Hi Mandy,

This looks good to me. There are a few places where a single call to Reference::get is replaced by multiple calls to Reference::refersTo, allowing the reference to get cleared in between, but as far as I could see that doesn't affect the overall logic which is still sound.
So LGTM!

best regards,
-- daniel

AlanBateman

Good to use this done in the same release as refersTo was added.

kimbarrett · 2020-12-04T18:12:36Z

src/java.base/share/classes/java/util/WeakHashMap.java

+
+        // then checks for equality
+        Object k = e.get();
+        return key == k || key.equals(k);


I think key == k is already covered by refersTo. But k could be null; checking for that here might be useful, to skip the call to equals in that case.

Good point on checking k != null. A cleaner check:

// then check for equality if the referent is not cleared Object k = e.get(); return k != null || key.equals(k);

You meant: return k != null && key.equals(k); right?

oops...yes.

plevart · 2020-12-04T19:47:17Z

src/java.management/share/classes/com/sun/jmx/mbeanserver/WeakIdentityHashMap.java

+            @SuppressWarnings("unchecked")
+            IdentityWeakReference<T> wr = (IdentityWeakReference<T>) o;
+            T got = get();
+            return got != null && wr.refersTo(got);
        }


Here you could pre-screen the get() with:

if (this.refersTo(null) != wr.refersTo(null)) return false;

In case one of the two WeakReference(s) is cleared and the other is not, they are not equal and you don't call this.get() in such case.

expunge is called at get, put, and remove. While there is a chance that a referent might be cleared, I will leave this as is and this patch simply replaces to use refersTo.

You're right. Where it would matter is in expunge() where the Map keys that get polled off the reference queue are all already cleared and when HashMap.remove(key) is called with such cleared key, it is this cleared key that is the target of key.equals(other) method invocation (and not vice versa), so it doesn't matter if .get() is called on such key as it is already cleared.

kevinrushforth

You have a typo that will cause a compilation error.

kevinrushforth · 2020-12-04T20:05:23Z

src/java.base/share/classes/java/util/WeakHashMap.java

@@ -428,7 +434,7 @@ public boolean containsKey(Object key) {
        Entry<K,V>[] tab = getTable();
        int index = indexFor(h, tab.length);
        Entry<K,V> e = tab[index];
-        while (e != null && !(e.hash == h && eq(k, e.get())))
+        while (e != null && !(e.hash == h && matchesKey(e, k))


This doesn't compile, which is why the checks from the GitHub actions build failed.

I caught that and it's in my local repo that I haven't pushed the commit. Sorry for not syncing my branch for review.

mlchung · 2020-12-06T00:07:41Z

/integrate

openjdk · 2020-12-06T00:08:47Z

@mlchung Since your change was applied there have been 8 commits pushed to the master branch:

78be334: 8242332: Add SHA3 support to SunPKCS11 provider
c4339c3: 8243614: Typo in ReentrantLock's Javadoc
d3ac1bf: 8198390: Test MultiResolutionDrawImageWithTransformTest.java fails when -esa is passed
51d325e: 8257633: Missing -mmacosx-version-min=X flag when linking libjvm
e27ea4d: 8257750: writeBuffer field of java.io.DataOutputStream should be final
dd0b945: 8257531: Super word not applied to a loop of simple Buffer operations
d76039d: 8257725: No throws of SSLHandshakeException
fcc8479: 8257724: Incorrect package of the linked class in BaseSSLSocketImpl

Your commit was automatically rebased without conflicts.

Pushed as commit 972bc3b.

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

kimbarrett and others added 12 commits October 2, 2020 21:59

new function

4226ae7

remove CMS comment

0ddc48f

expand test

2bcf123

change referent access

1773052

basic functional test

00b1b90

improve refersTo0 descriptions

28f2d1b

Merge branch 'refersto' of https://github.com/kimbarrett/openjdk-jdk …

bf1c337

…into refersTo

minor cleanup

0a3fd91

Merge branch 'master' of https://github.com/openjdk/jdk into refersTo

2740271

Merge branch 'master' of https://github.com/openjdk/jdk into refersTo

67c3108

8256167: Convert JDK use of to

c20e5c0

Merge branch 'master' of https://github.com/openjdk/jdk into refersTo

b1e645b

openjdk bot added the rfr Pull request is ready for review label Dec 3, 2020

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Dec 3, 2020

openjdk bot removed the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 3, 2020

sspitsyn approved these changes Dec 4, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 4, 2020

shipilev approved these changes Dec 4, 2020

View reviewed changes

dfuch approved these changes Dec 4, 2020

View reviewed changes

AlanBateman approved these changes Dec 4, 2020

View reviewed changes

update copyright end year and fixup grammatical nit

2766148

kimbarrett reviewed Dec 4, 2020

View reviewed changes

plevart reviewed Dec 4, 2020

View reviewed changes

kevinrushforth requested changes Dec 4, 2020

View reviewed changes

mlchung added 2 commits December 4, 2020 14:07

fix typo

a766b1e

revise matchsKey to check equality if the reference is not cleared

72947eb

kimbarrett approved these changes Dec 5, 2020

View reviewed changes

openjdk bot closed this Dec 6, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 6, 2020

mlchung deleted the refersTo branch March 25, 2021 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8256167: Convert JDK use of `Reference::get` to `Reference::refersTo` #1609

8256167: Convert JDK use of `Reference::get` to `Reference::refersTo` #1609

mlchung commented Dec 3, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 3, 2020

openjdk bot commented Dec 3, 2020

openjdk bot commented Dec 3, 2020

mlchung commented Dec 3, 2020

openjdk bot commented Dec 3, 2020

mlbridge bot commented Dec 3, 2020 •

edited

Loading

sspitsyn left a comment

openjdk bot commented Dec 4, 2020 •

edited

Loading

shipilev left a comment

shipilev Dec 4, 2020

mlchung Dec 4, 2020

dfuch left a comment

AlanBateman left a comment

kimbarrett Dec 4, 2020

mlchung Dec 4, 2020

plevart Dec 4, 2020

mlchung Dec 4, 2020

plevart Dec 4, 2020

mlchung Dec 4, 2020

plevart Dec 4, 2020 •

edited

Loading

kevinrushforth left a comment

kevinrushforth Dec 4, 2020

mlchung Dec 4, 2020 •

edited

Loading

mlchung commented Dec 6, 2020

openjdk bot commented Dec 6, 2020

8256167: Convert JDK use of Reference::get to Reference::refersTo #1609

8256167: Convert JDK use of Reference::get to Reference::refersTo #1609

Conversation

mlchung commented Dec 3, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 3, 2020

openjdk bot commented Dec 3, 2020

openjdk bot commented Dec 3, 2020

mlchung commented Dec 3, 2020

openjdk bot commented Dec 3, 2020

mlbridge bot commented Dec 3, 2020 • edited Loading

Webrevs

sspitsyn left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 4, 2020 • edited Loading

shipilev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfuch left a comment

Choose a reason for hiding this comment

AlanBateman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plevart Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlchung Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

mlchung commented Dec 6, 2020

openjdk bot commented Dec 6, 2020

8256167: Convert JDK use of `Reference::get` to `Reference::refersTo` #1609

8256167: Convert JDK use of `Reference::get` to `Reference::refersTo` #1609

mlchung commented Dec 3, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 3, 2020 •

edited

Loading

openjdk bot commented Dec 4, 2020 •

edited

Loading

plevart Dec 4, 2020 •

edited

Loading

mlchung Dec 4, 2020 •

edited

Loading