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
