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

[Peter Levart]

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