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 | tprinte...@twitter.com
<mailto:tprinte...@twitter.com>
On June 6, 2018 at 11:12:44 AM, Peter Levart (peter.lev...@gmail.com
<mailto:peter.lev...@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 | tprinte...@twitter.com
<mailto:tprinte...@twitter.com>
On June 6, 2018 at 9:38:05 AM, Alan Bateman
(alan.bate...@oracle.com <mailto:alan.bate...@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
