Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v3]
On Fri, 3 Sep 2021 12:01:12 GMT, Vladimir Ivanov wrote:
> > For the change of MethodHandle::asType to a final method, this needs a CSR.
>
> It is not allowed to extend/subclass `MethodHandle` outside
> `java.lang.invoke` package.
> So, the aforementioned change doesn't have any compatibility risks. Do I miss
> something important?
>
> ```
> /**
> * Package-private constructor for the method handle implementation
> hierarchy.
> * Method handle inheritance will be contained completely within
> * the {@code java.lang.invoke} package.
> */
> // @param type type (permanently assigned) of the new method handle
> /*non-public*/
> MethodHandle(MethodType type, LambdaForm form) {
> ```
Sorry for the late reply as I was on vacation and just return today.
Thanks for the clarification. I missed the fact that `MethodHandle` has no
public constructor and cannot be extended. This is fine.
-
PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v4]
On Fri, 3 Sep 2021 14:41:45 GMT, Vladimir Ivanov wrote: >> `MethodHandle.asTypeCache` keeps a strong reference to adapted >> `MethodHandle` and it can introduce a class loader leak through its >> `MethodType`. >> >> Proposed fix introduces a 2-level cache (1 element each) where 1st level can >> only contain `MethodHandle`s which are guaranteed to not introduce any >> dependencies on new class loaders compared to the original `MethodHandle`. >> 2nd level is backed by a `SoftReference` and is used as a backup when the >> result of `MethodHandle.asType()` conversion can't populate the higher level >> cache. >> >> The fix is based on [the >> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) >> made by Peter Levart @plevart back in 2015. >> >> Testing: tier1 - tier6 > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments Thanks for the reviews, Mandy, Paul, Peter, and Stuart. - PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v4]
On Fri, 3 Sep 2021 14:41:45 GMT, Vladimir Ivanov wrote: >> `MethodHandle.asTypeCache` keeps a strong reference to adapted >> `MethodHandle` and it can introduce a class loader leak through its >> `MethodType`. >> >> Proposed fix introduces a 2-level cache (1 element each) where 1st level can >> only contain `MethodHandle`s which are guaranteed to not introduce any >> dependencies on new class loaders compared to the original `MethodHandle`. >> 2nd level is backed by a `SoftReference` and is used as a backup when the >> result of `MethodHandle.asType()` conversion can't populate the higher level >> cache. >> >> The fix is based on [the >> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) >> made by Peter Levart @plevart back in 2015. >> >> Testing: tier1 - tier6 > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments I don't have a strong opinion on whether this should use a SoftReference or a WeakReference. (In fact, one might say that I have a phantom opinion.) The main thing was that the bug report mentioned WeakReference but the commit here uses SoftReferences, and I didn't see any discussion about this change. You gave some reasons above for why SoftReference is preferable, and that's ok with me. - PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v4]
On Fri, 3 Sep 2021 14:41:45 GMT, Vladimir Ivanov wrote: >> `MethodHandle.asTypeCache` keeps a strong reference to adapted >> `MethodHandle` and it can introduce a class loader leak through its >> `MethodType`. >> >> Proposed fix introduces a 2-level cache (1 element each) where 1st level can >> only contain `MethodHandle`s which are guaranteed to not introduce any >> dependencies on new class loaders compared to the original `MethodHandle`. >> 2nd level is backed by a `SoftReference` and is used as a backup when the >> result of `MethodHandle.asType()` conversion can't populate the higher level >> cache. >> >> The fix is based on [the >> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) >> made by Peter Levart @plevart back in 2015. >> >> Testing: tier1 - tier6 > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments Other MethodHandle/LambdaForm caches in `java.lang.invoke` deliberately rely on `SoftReference`s. (Only `MethodType` table uses `WeakReference`, but it is there for interning purposes.) MH adaptations are quite expensive (and may involve class loading), so it's beneficial to keep the result around for an extended period of time when circumstances allow. On the other hand, `MH.asType()` case is special because it can hold user classes around which makes effective memory footprint of cached value much higher. So, `WeakReference` would enable more prompt recycling of heap memory at the expense of higher cache miss rate. Personally, I'm in favor `SoftReference` here since it allows to get most of performance benefits out of the cache while preserving the correctness w.r.t. heap exhaustion. - PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v4]
On Fri, 3 Sep 2021 14:41:45 GMT, Vladimir Ivanov wrote: >> `MethodHandle.asTypeCache` keeps a strong reference to adapted >> `MethodHandle` and it can introduce a class loader leak through its >> `MethodType`. >> >> Proposed fix introduces a 2-level cache (1 element each) where 1st level can >> only contain `MethodHandle`s which are guaranteed to not introduce any >> dependencies on new class loaders compared to the original `MethodHandle`. >> 2nd level is backed by a `SoftReference` and is used as a backup when the >> result of `MethodHandle.asType()` conversion can't populate the higher level >> cache. >> >> The fix is based on [the >> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) >> made by Peter Levart @plevart back in 2015. >> >> Testing: tier1 - tier6 > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments Did we want this to use a soft reference or a weak reference? The original bug report mentions weak references, and the typical approach for preventing caches from holding onto things unnecessarily is to use weak references. I haven't thought through the implications of using soft instead of weak references. (Sorry if I missed any discussion on this issue.) - PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v4]
On Fri, 3 Sep 2021 14:41:45 GMT, Vladimir Ivanov wrote: >> `MethodHandle.asTypeCache` keeps a strong reference to adapted >> `MethodHandle` and it can introduce a class loader leak through its >> `MethodType`. >> >> Proposed fix introduces a 2-level cache (1 element each) where 1st level can >> only contain `MethodHandle`s which are guaranteed to not introduce any >> dependencies on new class loaders compared to the original `MethodHandle`. >> 2nd level is backed by a `SoftReference` and is used as a backup when the >> result of `MethodHandle.asType()` conversion can't populate the higher level >> cache. >> >> The fix is based on [the >> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) >> made by Peter Levart @plevart back in 2015. >> >> Testing: tier1 - tier6 > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments Marked as reviewed by plevart (Reviewer). This looks good to me now. - PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v3]
On Fri, 3 Sep 2021 12:51:13 GMT, Peter Levart wrote:
>> Vladimir Ivanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address review comments
>
> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 877:
>
>> 875: }
>> 876: if (asTypeSoftCache != null) {
>> 877: atc = asTypeSoftCache.get();
>
> NPE is possible here too! asTypeSoftCache is a non-volatile field which is
> read twice. First time in the if (...) condition, 2nd time in the line that
> de-references it to call .get(). This is a data-race since concurrent thread
> may be setting this field from null to non-null. Those two reads may get
> reordered. 1st read may return non-null while 2nd may return null. This can
> be avoided if the field is read just once by introducing a local variable to
> store its value.
Fixed.
> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 878:
>
>> 876: if (asTypeSoftCache != null) {
>> 877: atc = asTypeSoftCache.get();
>> 878: if (newType == atc.type) {
>
> NPE is possible here! act can be null as it is a result of SoftReference::get
Good catch! Fixed.
> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 933:
>
>> 931: }
>> 932:
>> 933: /* Returns true when {@code loader} keeps {@code mt} either
>> directly or indirectly through the loader delegation chain. */
>
> Well, to be precise, loader can't keep mt alive. It would be better to say
> "keeps mt components alive" ...
Fixed.
> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 948:
>
>> 946: if (isBuiltinLoader(defLoader)) {
>> 947: return true; // built-in loaders are always reachable
>> 948: }
>
> No need for special case here. isAncestorLoaderOf(defLoader, loader) already
> handles this case.
Though the check is redundant, I find the current version clearer.
-
PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v4]
> `MethodHandle.asTypeCache` keeps a strong reference to adapted `MethodHandle` > and it can introduce a class loader leak through its `MethodType`. > > Proposed fix introduces a 2-level cache (1 element each) where 1st level can > only contain `MethodHandle`s which are guaranteed to not introduce any > dependencies on new class loaders compared to the original `MethodHandle`. > 2nd level is backed by a `SoftReference` and is used as a backup when the > result of `MethodHandle.asType()` conversion can't populate the higher level > cache. > > The fix is based on [the > work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) > made by Peter Levart @plevart back in 2015. > > Testing: tier1 - tier6 Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5246/files - new: https://git.openjdk.java.net/jdk/pull/5246/files/7a87aee3..365dd454 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5246&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5246&range=02-03 Stats: 5 lines in 1 file changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5246.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5246/head:pull/5246 PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v3]
On Thu, 2 Sep 2021 11:35:52 GMT, Vladimir Ivanov wrote:
>> `MethodHandle.asTypeCache` keeps a strong reference to adapted
>> `MethodHandle` and it can introduce a class loader leak through its
>> `MethodType`.
>>
>> Proposed fix introduces a 2-level cache (1 element each) where 1st level can
>> only contain `MethodHandle`s which are guaranteed to not introduce any
>> dependencies on new class loaders compared to the original `MethodHandle`.
>> 2nd level is backed by a `SoftReference` and is used as a backup when the
>> result of `MethodHandle.asType()` conversion can't populate the higher level
>> cache.
>>
>> The fix is based on [the
>> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/)
>> made by Peter Levart @plevart back in 2015.
>>
>> Testing: tier1 - tier6
>
> Vladimir Ivanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address review comments
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 877:
> 875: }
> 876: if (asTypeSoftCache != null) {
> 877: atc = asTypeSoftCache.get();
NPE is possible here too! asTypeSoftCache is a non-volatile field which is read
twice. First time in the if (...) condition, 2nd time in the line that
de-references it to call .get(). This is a data-race since concurrent thread
may be setting this field from null to non-null. Those two reads may get
reordered. 1st read may return non-null while 2nd may return null. This can be
avoided if the field is read just once by introducing a local variable to store
its value.
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 878:
> 876: if (asTypeSoftCache != null) {
> 877: atc = asTypeSoftCache.get();
> 878: if (newType == atc.type) {
NPE is possible here! act can be null as it is a result of SoftReference::get
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 933:
> 931: }
> 932:
> 933: /* Returns true when {@code loader} keeps {@code mt} either directly
> or indirectly through the loader delegation chain. */
Well, to be precise, loader can't keep mt alive. It would be better to say
"keeps mt components alive" ...
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 948:
> 946: if (isBuiltinLoader(defLoader)) {
> 947: return true; // built-in loaders are always reachable
> 948: }
No need for special case here. isAncestorLoaderOf(defLoader, loader) already
handles this case.
-
PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v3]
On Fri, 3 Sep 2021 00:09:17 GMT, Mandy Chung wrote:
> For the change of MethodHandle::asType to a final method, this needs a CSR.
It is not allowed to extend/subclass `MethodHandle` outside `java.lang.invoke`
package.
So, the aforementioned change doesn't have any compatibility risks. Do I miss
something important?
/**
* Package-private constructor for the method handle implementation
hierarchy.
* Method handle inheritance will be contained completely within
* the {@code java.lang.invoke} package.
*/
// @param type type (permanently assigned) of the new method handle
/*non-public*/
MethodHandle(MethodType type, LambdaForm form) {
-
PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v3]
On Thu, 2 Sep 2021 11:35:52 GMT, Vladimir Ivanov wrote: >> `MethodHandle.asTypeCache` keeps a strong reference to adapted >> `MethodHandle` and it can introduce a class loader leak through its >> `MethodType`. >> >> Proposed fix introduces a 2-level cache (1 element each) where 1st level can >> only contain `MethodHandle`s which are guaranteed to not introduce any >> dependencies on new class loaders compared to the original `MethodHandle`. >> 2nd level is backed by a `SoftReference` and is used as a backup when the >> result of `MethodHandle.asType()` conversion can't populate the higher level >> cache. >> >> The fix is based on [the >> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) >> made by Peter Levart @plevart back in 2015. >> >> Testing: tier1 - tier6 > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments This looks good to me. For the change of `MethodHandle::asType` to a final method, this needs a CSR. Is this spec change intentional? I wonder if `MethodHandle` should be a sealed class instead. In any case, maybe you can consider the spec change as a separate issue. - PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v2]
On Wed, 1 Sep 2021 17:28:37 GMT, Mandy Chung wrote:
>> Vladimir Ivanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address review comments
>
> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 926:
>
>> 924: /* Returns true when {@code loader} keeps {@code cls} either
>> directly or indirectly through the loader delegation chain. */
>> 925: private static boolean keepsAlive(Class cls, ClassLoader loader)
>> {
>> 926: return keepsAlive(cls.getClassLoader(), loader);
>
> Suggestion:
>
> ClassLoader defLoader = cls.getClassLoader();
> if (isBuiltinLoader(defLoader)) {
> return true; // built-in loaders are always reachable
> }
> return keepsAlive(defLoader, loader);
>
>
> I think it's clearer to check if `cls` is not defined by any builtin loader
> here and then check if `loader` keeps `cls` alive.
>
> So `keepsAlive(ClassLoader loader1, ClassLoader loader2)` is not needed and
> replace line 935 and 940 to `keepsAlive(Class, ClassLoader)` instead.
Sounds good.
Incorporated your suggestions along with some minor refactorings.
-
PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v3]
> `MethodHandle.asTypeCache` keeps a strong reference to adapted `MethodHandle` > and it can introduce a class loader leak through its `MethodType`. > > Proposed fix introduces a 2-level cache (1 element each) where 1st level can > only contain `MethodHandle`s which are guaranteed to not introduce any > dependencies on new class loaders compared to the original `MethodHandle`. > 2nd level is backed by a `SoftReference` and is used as a backup when the > result of `MethodHandle.asType()` conversion can't populate the higher level > cache. > > The fix is based on [the > work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) > made by Peter Levart @plevart back in 2015. > > Testing: tier1 - tier6 Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5246/files - new: https://git.openjdk.java.net/jdk/pull/5246/files/b2c0..7a87aee3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5246&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5246&range=01-02 Stats: 41 lines in 1 file changed: 15 ins; 18 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/5246.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5246/head:pull/5246 PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v2]
On Wed, 1 Sep 2021 16:23:46 GMT, Vladimir Ivanov wrote:
>> `MethodHandle.asTypeCache` keeps a strong reference to adapted
>> `MethodHandle` and it can introduce a class loader leak through its
>> `MethodType`.
>>
>> Proposed fix introduces a 2-level cache (1 element each) where 1st level can
>> only contain `MethodHandle`s which are guaranteed to not introduce any
>> dependencies on new class loaders compared to the original `MethodHandle`.
>> 2nd level is backed by a `SoftReference` and is used as a backup when the
>> result of `MethodHandle.asType()` conversion can't populate the higher level
>> cache.
>>
>> The fix is based on [the
>> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/)
>> made by Peter Levart @plevart back in 2015.
>>
>> Testing: tier1 - tier6
>
> Vladimir Ivanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address review comments
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 926:
> 924: /* Returns true when {@code loader} keeps {@code cls} either
> directly or indirectly through the loader delegation chain. */
> 925: private static boolean keepsAlive(Class cls, ClassLoader loader) {
> 926: return keepsAlive(cls.getClassLoader(), loader);
Suggestion:
ClassLoader defLoader = cls.getClassLoader();
if (isBuiltinLoader(defLoader)) {
return true; // built-in loaders are always reachable
}
return keepsAlive(defLoader, loader);
I think it's clearer to check if `cls` is not defined by any builtin loader
here and then check if `loader` keeps `cls` alive.
So `keepsAlive(ClassLoader loader1, ClassLoader loader2)` is not needed and
replace line 935 and 940 to `keepsAlive(Class, ClassLoader)` instead.
-
PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading
On Wed, 25 Aug 2021 09:31:51 GMT, Vladimir Ivanov wrote: > `MethodHandle.asTypeCache` keeps a strong reference to adapted `MethodHandle` > and it can introduce a class loader leak through its `MethodType`. > > Proposed fix introduces a 2-level cache (1 element each) where 1st level can > only contain `MethodHandle`s which are guaranteed to not introduce any > dependencies on new class loaders compared to the original `MethodHandle`. > 2nd level is backed by a `SoftReference` and is used as a backup when the > result of `MethodHandle.asType()` conversion can't populate the higher level > cache. > > The fix is based on [the > work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) > made by Peter Levart @plevart back in 2015. > > Testing: tier1 - tier6 Thanks for the reviews, Paul and Mandy. What do you think about the latest version? - PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading [v2]
> `MethodHandle.asTypeCache` keeps a strong reference to adapted `MethodHandle` > and it can introduce a class loader leak through its `MethodType`. > > Proposed fix introduces a 2-level cache (1 element each) where 1st level can > only contain `MethodHandle`s which are guaranteed to not introduce any > dependencies on new class loaders compared to the original `MethodHandle`. > 2nd level is backed by a `SoftReference` and is used as a backup when the > result of `MethodHandle.asType()` conversion can't populate the higher level > cache. > > The fix is based on [the > work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) > made by Peter Levart @plevart back in 2015. > > Testing: tier1 - tier6 Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5246/files - new: https://git.openjdk.java.net/jdk/pull/5246/files/87154817..b2c0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5246&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5246&range=00-01 Stats: 13 lines in 1 file changed: 4 ins; 1 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/5246.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5246/head:pull/5246 PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading
On Thu, 26 Aug 2021 17:57:26 GMT, Paul Sandoz wrote:
>> `MethodHandle.asTypeCache` keeps a strong reference to adapted
>> `MethodHandle` and it can introduce a class loader leak through its
>> `MethodType`.
>>
>> Proposed fix introduces a 2-level cache (1 element each) where 1st level can
>> only contain `MethodHandle`s which are guaranteed to not introduce any
>> dependencies on new class loaders compared to the original `MethodHandle`.
>> 2nd level is backed by a `SoftReference` and is used as a backup when the
>> result of `MethodHandle.asType()` conversion can't populate the higher level
>> cache.
>>
>> The fix is based on [the
>> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/)
>> made by Peter Levart @plevart back in 2015.
>>
>> Testing: tier1 - tier6
>
> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 953:
>
>> 951:
>> 952: /* Determine whether {@code descendant} keeps {@code ancestor}
>> alive through the loader delegation chain. */
>> 953: private static boolean keepsAlive(ClassLoader ancestor, ClassLoader
>> descendant) {
>
> Might be clearer to name the method by what it is e.g. isAncestor
> // Returns true if ancestor can be found descendant's delegation chain.
This method is not exactly doing `isAncestor` check. It returns true if
`ancestor` is a builtin loader even it's not an ancestor of `descendent`. I
agree that it would be helpful if the method is named by what it is.
Maybe naming it `isAncestor` but move the `isSystemLoader(ancestor)` check out
to the caller. Just a thought.
-
PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading
On Wed, 25 Aug 2021 09:31:51 GMT, Vladimir Ivanov wrote:
> `MethodHandle.asTypeCache` keeps a strong reference to adapted `MethodHandle`
> and it can introduce a class loader leak through its `MethodType`.
>
> Proposed fix introduces a 2-level cache (1 element each) where 1st level can
> only contain `MethodHandle`s which are guaranteed to not introduce any
> dependencies on new class loaders compared to the original `MethodHandle`.
> 2nd level is backed by a `SoftReference` and is used as a backup when the
> result of `MethodHandle.asType()` conversion can't populate the higher level
> cache.
>
> The fix is based on [the
> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/)
> made by Peter Levart @plevart back in 2015.
>
> Testing: tier1 - tier6
Thanks for fixing this. JEP 416 depends on this.
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 966:
> 964: }
> 965:
> 966: private static boolean isSystemLoader(ClassLoader loader) {
These are builtin loaders. This method may be better to rename to
`isBuiltinLoader`.
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading
On Wed, 25 Aug 2021 09:31:51 GMT, Vladimir Ivanov wrote:
> `MethodHandle.asTypeCache` keeps a strong reference to adapted `MethodHandle`
> and it can introduce a class loader leak through its `MethodType`.
>
> Proposed fix introduces a 2-level cache (1 element each) where 1st level can
> only contain `MethodHandle`s which are guaranteed to not introduce any
> dependencies on new class loaders compared to the original `MethodHandle`.
> 2nd level is backed by a `SoftReference` and is used as a backup when the
> result of `MethodHandle.asType()` conversion can't populate the higher level
> cache.
>
> The fix is based on [the
> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/)
> made by Peter Levart @plevart back in 2015.
>
> Testing: tier1 - tier6
Looks good.
I guess it is not that common for the soft ref to get instantiated i.e. for the
case of the ~common class loader of `type`, MTC say, and the ~common classoader
of `newType`, NMTC say, then NMTC is not an ancestor of MTC.
It's possible that `asTypeCache` and `asTypeSoftCache` could both be non-null
i.e. we don't null out one of them, buy does not seem a problem.
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 869:
> 867: }
> 868: at = asTypeUncached(newType);
> 869: return setAsTypeCache(at);
The following may be a little clearer
Suggestion:
MethodHandle at = asTypeCached(newType);
return at != null
? at
: setAsTypeCache(asTypeUncached(newType));
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 953:
> 951:
> 952: /* Determine whether {@code descendant} keeps {@code ancestor} alive
> through the loader delegation chain. */
> 953: private static boolean keepsAlive(ClassLoader ancestor, ClassLoader
> descendant) {
Might be clearer to name the method by what it is e.g. isAncestor
// Returns true if ancestor can be found descendant's delegation chain.
-
Marked as reviewed by psandoz (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5246
RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading
`MethodHandle.asTypeCache` keeps a strong reference to adapted `MethodHandle` and it can introduce a class loader leak through its `MethodType`. Proposed fix introduces a 2-level cache (1 element each) where 1st level can only contain `MethodHandle`s which are guaranteed to not introduce any dependencies on new class loaders compared to the original `MethodHandle`. 2nd level is backed by a `SoftReference` and is used as a backup when the result of `MethodHandle.asType()` conversion can't populate the higher level cache. The fix is based on [the work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) made by Peter Levart @plevart back in 2015. Testing: tier1 - tier6 - Commit messages: - Remove the assert - Introduce 2nd level MethodHandle.asType cache - keepsAlive logic Changes: https://git.openjdk.java.net/jdk/pull/5246/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5246&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8078641 Stats: 108 lines in 2 files changed: 87 ins; 3 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/5246.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5246/head:pull/5246 PR: https://git.openjdk.java.net/jdk/pull/5246
