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

[Peter Levart]

Hi Tony,

Thanks for taking a look. Just a couple of comments inline...

On 06/06/18 22:38, Tony Printezis wrote:
>>
>> - instead of overriding initialValue(), ThreadLocal.setInitialValue() 
>> calls TerminatingThreadLocal.register() at the right moment
>
>
> Thanks. I much prefer not having to introduce calculateInitialValue().
>
> One extra suggestion: Given you introduced a call to 
> TerminatingThreadLocal from ThreadLocal, would it make sense to maybe 
> do the same for set() and remove() (you’d have to add an appropriate 
> check in unregister) and not override them in TerminatingTreadLocal? I 
> totally get why you might not want to do that (an extra check when a 
> ThreadLocal is not the Terminating one). I’m OK either way.
>

Yes, precisely. My 1st version did just that, but since there are usages 
of ThreadLocal out there that are very performance sensitive, I opted 
for an approach where there is zero performance regression for 
non-TerminatingThreadLocal(s) when calling set() or remove(). A 
call-back from setInitialValue is not so problematic as it usually 
occurs just once per thread, but I can imagine a scenario where calls to 
get() and set() occur very rapidly.

>>
>> 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.
>
>
> Something like:
>
> if (tl.isPresent()) {
>
>    T val = t.get();
>
>    ….
>
> }
>
> will do two look-ups if the value exists. However, that’s better than 
> allocating unnecessarily. So, I’ll take isPresent() over not having a 
> way to check whether a value exists.
>

Right, and in our scenario, it is just isPresent() that is called for 
every termination of every thread (REGISTRY.isPresent()). .get() is then 
called only when there's actually something to do.

>
> Thumbs up from me.
>

Let's just wait for a day or two to see whether the discussion on 
concurrency-interest gives us any additional ideas. Currently I'm 
thinking of proposing the addition of isPresent() API. As far as this 
RFR is concerned, I'm consequently promoting the latest webrev.04.

So any comments on that one? Alan?

Regards, Peter