RFR: 8198540: Dynalink leaks memory when generating type converters [v3]

[Peter Levart]

On Sat, 2 Jan 2021 16:28:19 GMT, Johannes Kuhn <github.com+652983+DasBrain at openjdk.org> wrote:

>> Attila Szegedi has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   jtreg runs with assertions on, so using the assert keyword is sufficient
>
> Just a small suggestion using `MethodHandles.empty` instead of `MethodHandles.constant().asType().dropArguments()`.

Hi Attila,

This looks good.

If I understand correctly, your `BiClassValue` is a holder for a `BiFunction` and a `BiClassValueRoot` which is a `ClassValue<BiClassValues>`. The `BiClassValues` contains two lazily constructed Map(s): `forward` and `reverse`. You then employ a logic where you either use the `forward` map obtained from c1 if c1.classLoader sees c2.classLoader or `backward` map obtained from c2 if c2.classLoader sees c1.classLoader or you don't employ caching if c1.classLoader and c2.classLoader are unrelated.
The need for two Maps is probably because the two Class components of the "bi-key" are ordered, right? So `BiClassValue bcv = ...;` `bcv.get(c1, c2)` is not the same as `bcv.get(c2, c1)` ....
Alternatively you could have a `BiClassValue` with two embedded `ClassValue`(s):

final CVRoot<T> forward = new CVRoot<>();
final CVRoot<T> reverse = new CVRoor<>();

static class CVRoot<T> extends ClassValue<CHMCL<T>> {
  public CHMCL<T> get(Class<?> clazz) { return new CHMCL<>(getClassLoader(clazz)); }
}

static class CHMCL<T> extends ConcurrentHashMap<Class<?>, T> {
  final ClassLoader classLoader;
  CHMCL(ClassLoader cl) { classLoader = cl; }
}

...with that you could get rid of the intermediary BiClassValues object. It would be replaced with a ConcurrentHashMap subclass that would contain a field to hold the cached ClassLoader of the c1/c2. One de-reference less...

As for the main logic, it would be similar to what you have now. Or it could be different. I wonder what is the performance of canReferenceDirectly(). If you used SharedSecrets to obtain a ClassLoader of a Class without security checks (and thus overhead), you could perhaps evaluate the canReferenceDirectly() before lookups so you would not needlessly trigger initialization of ClassValue(s) that don't need to get initialized. 

WDYT?

-------------

PR: https://git.openjdk.java.net/jdk/pull/1918