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

[Tony Printezis]

Peter,

Inline.


—————
Tony Printezis | @TonyPrintezis | tprintezis at twitter.com


On June 7, 2018 at 5:21:43 AM, Peter Levart (peter.levart at gmail.com) wrote:

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.


Yeah, I agree that this is a good trade-off given how the code is currently
structured.





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.


Yes. I’m not worried about that at all.




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.


Again, #ThumbsUp from me. Thanks,


Tony



So any comments on that one? Alan?

Regards, Peter