RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit

[Peter Levart]

Update: the discussion on concurrent-interest about possible future 
addition of public ThreadLocal.getIfPresent() or ThreadLocal.isPresent() 
has died out, but it nevertheless reached a point where 
ThreadLocal.isPresent() was found the least problematic. So I would like 
to get further with this proposal using the last webrev.04 version of 
the patch that uses ThreadLocal.isPresent() internally - still 
package-private at the moment.

Here's the webrev (unchanged from the day it was published):

http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.04/

Would this version be OK?

Regards, Peter


On 06/06/18 20:55, Peter Levart wrote:
> Ok, here's next webrev:
>
> http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.03/
>
> The changes from webrev.02 are:
>
> - renamed JdkThreadLocal -> TerminatingThreadLocal
> - instead of overriding initialValue(), ThreadLocal.setInitialValue() 
> calls TerminatingThreadLocal.register() at the right moment
> - TerminatingThreadLocal.threadTerminated() now takes a (T value) 
> parameter
> - TerminatingThreadLocal.REGISTRY uses IdentityHashSet instead of 
> HashSet (if .equals()/.hashCode() are overriden in some 
> TerminatingThreadLocal subclass)
>
> David Lloyd has commented on concurrency-interest about 
> ThreadLocal.getIfPresent(). There is a concern that such new method 
> would be inconsistent with what ThreadLocal.get() returns from 
> existing ThreadLocal subclasses that override .get() and possibly 
> transform the value returned from super.get() on the way.
>
> An alternative to "T getIfPresent()" is a "boolean isPresent()" 
> method. Even if it remains package-private, with such method 
> TerminatingThreadLocal could be implemented as:
>
> http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.04/
>
> If TreadLocal.isPresent() was made public, the code could be further 
> simplified.
>
>
> Regards, Peter
>
>
> On 06/06/18 18:58, Peter Levart wrote:
>>
>>
>> On 06/06/18 17:41, Tony Printezis wrote:
>>> Peter,
>>>
>>> You’re totally right re: JdkThreadLocal::initialValue being final 
>>> and cannot be overridden (I had completely missed the final keyword 
>>> when I first looked at the webrev). But it’d be nice to have the 
>>> same API in both versions of ThreadLocal. I assume you didn’t want 
>>> to override get() since you only wanted to update the REGISTRY the 
>>> first time get() is called by a thread. If getIfPresent() is 
>>> exposed, would something like the following work?
>>>
>>> @Override
>>> public T get() {
>>>   final T value = getIfPresent();
>>>   if (value != null) {
>>>       return value;
>>>   }
>>>   REGISTRY.get().add(this);
>>>   return super.get();
>>> }
>>
>> This would work, but if mapped value was 'null', it would keep 
>> calling REGISTRY.get().add(this) for each get(). Logically correct, 
>> but with useless overhead.
>>
>>  Let me try to do something that might satisfy you... (in next webrev).
>>
>>>
>>> One more question re: getIfPresent() (and maybe I’m overthinking 
>>> this): It returns null to indicate that a value is not present. 
>>> Isn’t null a valid ThreadLocal value? Would using an Optional here 
>>> be more appropriate?
>>
>> The problem with Optional is that it does not provide an additional 
>> value. Optional.empty() is a replacement for null. There's no 
>> Optional.of(null). So what would getIfPresent() return when there was 
>> a mapping present, but that mapping was null?
>>
>> Regards, Peter
>>
>>>
>>> Tony
>>>
>>> —————
>>> Tony Printezis | @TonyPrintezis | tprintezis at twitter.com 
>>> <mailto:tprintezis at twitter.com>
>>>
>>>
>>> On June 6, 2018 at 11:12:44 AM, Peter Levart (peter.levart at gmail.com 
>>> <mailto:peter.levart at gmail.com>) wrote:
>>>
>>>> Hi Tony, Alan,
>>>>
>>>> On 06/06/2018 04:37 PM, Tony Printezis wrote:
>>>>> Alan,
>>>>>
>>>>> Thanks for your thoughts!
>>>>>
>>>>> Peter,
>>>>>
>>>>> Any chance of taking the two suggestions I made in an earlier 
>>>>> e-mail into account?
>>>>
>>>> Right, I'll prepare new webrev with that shortly.
>>>>
>>>>>
>>>>> a) It’d be nice if users can override initialValue(), like when 
>>>>> using the standard ThreadLocal class, instead of 
>>>>> calculateInitialValue(). (I can’t come up with a clean solution on 
>>>>> how to do that, though. I’ll think about it for a bit.)
>>>>
>>>> Your concern was that users might accidentally override 
>>>> initialValue() instead of calculateInitialValue(), if I understood 
>>>> you correctly. If that was the concern, they can't, because it is 
>>>> final. If the concern was that users would want to override 
>>>> initialValue() because they are used to do so, then perhaps a 
>>>> javadoc on final initialiValue() pointing to 
>>>> calculateInitialValue() might help them. Do you agree? This is 
>>>> currently internal API and consistent naming is not a big concern.
>>>>
>>>>> b) It’d be very helpful to pass T value to threadTerminated(), as 
>>>>> I cannot imagine many use-cases where the value will not be needed.
>>>>
>>>> Agree. Will include that in new webrev.
>>>>
>>>>>
>>>>> Re: renaming JdkThreadLocal: ThreadLocalWithExitHooks?
>>>>
>>>> Hm. Exit Hooks are already something that is used in JVM (Threads 
>>>> started when VM is about to exit), so this might be confusing for 
>>>> someone.
>>>>
>>>> - DisposableThreadLocal
>>>> - ThreadLocalWithCleanup
>>>>
>>>> And my favorite:
>>>>
>>>> - TerminatingThreadLocal
>>>>
>>>>
>>>>>
>>>>> Re: exposing getIfPresent() : Yes! Pretty please! :-) This will be 
>>>>> very helpful and can avoid completely unnecessary allocations.
>>>>
>>>> I agree that this would be generally useful functionality. Might be 
>>>> good to ask for opinion on concurrency-interest mailing list first. 
>>>> Will do that.
>>>>
>>>> Regards, Peter
>>>>
>>>>>
>>>>> Tony
>>>>>
>>>>>
>>>>> —————
>>>>> Tony Printezis | @TonyPrintezis | tprintezis at twitter.com 
>>>>> <mailto:tprintezis at twitter.com>
>>>>>
>>>>>
>>>>> On June 6, 2018 at 9:38:05 AM, Alan Bateman 
>>>>> (alan.bateman at oracle.com <mailto:alan.bateman at oracle.com>) wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 30/05/2018 22:16, Peter Levart wrote:
>>>>>> > I thought there would be some hint from Alan about which of the two
>>>>>> > paths we should take for more refinement.
>>>>>> >
>>>>>> > The Tony's:
>>>>>> >
>>>>>> > http://cr.openjdk.java.net/~tonyp/8202788/webrev.1/ 
>>>>>> <http://cr.openjdk.java.net/%7Etonyp/8202788/webrev.1/>
>>>>>> >
>>>>>> > Or the Alan's with my mods:
>>>>>> >
>>>>>> > 
>>>>>> http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.02/ 
>>>>>> <http://cr.openjdk.java.net/%7Eplevart/jdk-dev/DBBCache_Cleanup/webrev.02/>
>>>>>> >
>>>>>> Finally getting back to this one.
>>>>>>
>>>>>> I don't think the two approaches are all that different now. Tony's
>>>>>> point on the number of hooks vs. number of locals was an 
>>>>>> important point
>>>>>> but with Peter's update to use a thread local registry then I 
>>>>>> think we
>>>>>> have easy to maintain solution in the DBBCache_Cleanup/webrev.02 
>>>>>> patch.
>>>>>> So I think I prefer that approach. We need to better name for
>>>>>> "JdkThreadLocal", something to indicate that it holds resources or it
>>>>>> notified when a thread terminates.
>>>>>>
>>>>>> Also along the way, we touched on exposing getIfPresent and we should
>>>>>> look at that again. If it is expose then it fits well with the second
>>>>>> approach too.
>>>>>>
>>>>>> -Alan
>>>>
>>
>