Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo` [v2]
On Fri, 4 Dec 2020 22:38:24 GMT, Mandy Chung wrote: >> 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. > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - revise matchsKey to check equality if the reference is not cleared > - fix typo > - update copyright end year and fixup grammatical nit > - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo > - 8256167: Convert JDK use of to > - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo > - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo > - minor cleanup > - Merge branch 'refersto' of https://github.com/kimbarrett/openjdk-jdk into > refersTo > - improve refersTo0 descriptions > - ... and 5 more: > https://git.openjdk.java.net/jdk/compare/d8ac76fa...72947eb3 Marked as reviewed by kbarrett (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo` [v2]
> 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. Mandy Chung has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - revise matchsKey to check equality if the reference is not cleared - fix typo - update copyright end year and fixup grammatical nit - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo - 8256167: Convert JDK use of to - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo - minor cleanup - Merge branch 'refersto' of https://github.com/kimbarrett/openjdk-jdk into refersTo - improve refersTo0 descriptions - ... and 5 more: https://git.openjdk.java.net/jdk/compare/d8ac76fa...72947eb3 - Changes: https://git.openjdk.java.net/jdk/pull/1609/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1609=01 Stats: 60 lines in 12 files changed: 7 ins; 11 del; 42 mod Patch: https://git.openjdk.java.net/jdk/pull/1609.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1609/head:pull/1609 PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Fri, 4 Dec 2020 20:14:13 GMT, Peter Levart wrote: >> 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. - PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Fri, 4 Dec 2020 20:16:14 GMT, Mandy Chung wrote: >> src/java.management/share/classes/com/sun/jmx/mbeanserver/WeakIdentityHashMap.java >> line 127: >> >>> 125: T got = get(); >>> 126: return got != null && wr.refersTo(got); >>> 127: } >> >> 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. - PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Fri, 4 Dec 2020 20:05:23 GMT, Kevin Rushforth wrote: >> 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. > > src/java.base/share/classes/java/util/WeakHashMap.java line 437: > >> 435: int index = indexFor(h, tab.length); >> 436: Entry e = tab[index]; >> 437: 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. - PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Fri, 4 Dec 2020 19:47:17 GMT, Peter Levart wrote: >> 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. > > src/java.management/share/classes/com/sun/jmx/mbeanserver/WeakIdentityHashMap.java > line 127: > >> 125: T got = get(); >> 126: return got != null && wr.refersTo(got); >> 127: } > > 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`. - PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Fri, 4 Dec 2020 20:09:01 GMT, Mandy Chung wrote: >> src/java.base/share/classes/java/util/WeakHashMap.java line 293: >> >>> 291: // then checks for equality >>> 292: Object k = e.get(); >>> 293: 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? - PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Fri, 4 Dec 2020 18:12:36 GMT, Kim Barrett wrote: >> 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. > > src/java.base/share/classes/java/util/WeakHashMap.java line 293: > >> 291: // then checks for equality >> 292: Object k = e.get(); >> 293: 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); - PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung wrote: > 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. You have a typo that will cause a compilation error. src/java.base/share/classes/java/util/WeakHashMap.java line 437: > 435: int index = indexFor(h, tab.length); > 436: Entry e = tab[index]; > 437: while (e != null && !(e.hash == h && matchesKey(e, k)) This doesn't compile, which is why the checks from the GitHub actions build failed. - Changes requested by kcr (Author). PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung wrote: > 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. src/java.management/share/classes/com/sun/jmx/mbeanserver/WeakIdentityHashMap.java line 127: > 125: T got = get(); > 126: return got != null && wr.refersTo(got); > 127: } 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. - PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung wrote: > 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. src/java.base/share/classes/java/util/WeakHashMap.java line 293: > 291: // then checks for equality > 292: Object k = e.get(); > 293: 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. - PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Fri, 4 Dec 2020 09:19:23 GMT, Aleksey Shipilev wrote: >> 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. > > src/java.base/share/classes/java/util/WeakHashMap.java line 291: > >> 289: if (e.refersTo(key)) return true; >> 290: >> 291: // 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. - PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung wrote: > 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. Good to use this done in the same release as refersTo was added. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung wrote: > 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. 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 - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung wrote: > 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. Replacements look fine to me. src/java.base/share/classes/java/util/WeakHashMap.java line 291: > 289: if (e.refersTo(key)) return true; > 290: > 291: // 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`? - Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1609
