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 [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 [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
