Hi Tony,
If we anticipate only small number of JdkThreadLocal(s) (there will be
only one for the start) then this is a plausible solution. Then perhaps
this limitation should be written somewhere in the JdkThreadLocal
javadoc so that one day somebody will not be tempted to use
JdkThreadLocal(s) en masse. Let's say there will be a few more
JdkThreadLocal(s) in the future. Are we willing to pay for a few lookups
into a ThreadLocalMap at each and every thread's exit even though such
thread did not register a mapping for any JdkThreadLocal? Is an
additional reference field in each and every ThreadLocalMap (one per
Thread that uses thread locals) a bigger price to pay? I don't know.
Will let others comment on this.
Otherwise the code looks good. Just a couple of observations:
Since private static method JdkThreadLocal.add is only called from
JdkThreadLocal constructor with just constructed instance (this),
there's no possibility for it to be called twice or more times with the
same instance. The check for duplicates could go away then, right?
You keep an array of Entry objects which are just wrappers for
JdkThreadLocal objects. Are you already planning for Entry to become a
WeakReference? Otherwise you could just keep JdkThreadLocal objects in
the array directly.
Regards, Peter
On 05/17/18 20:25, Tony Printezis wrote:
Hi all,
I have a new version of the code for your consideration:
http://cr.openjdk.java.net/~tonyp/8202788/webrev.1/
<http://cr.openjdk.java.net/%7Etonyp/8202788/webrev.1/>
I basically combined our two approaches. The usage is as Alan had
proposed it: Users have to use JdkThreadLocal (which is only available
within java.base) and override threadTerminated(). However, I keep
track of JdkThreadLocal instances globally (as I did before) and not
per-thread. This way we don’t need to add any unnecessary complexity
to ThreadLocalMap.
Currently, I don’t allow entries to be purged if the JdkThreadLocal
instance becomes otherwise unreachable. I can easily add that
functionality if needed (I can use WeakReferences for that). However,
for the uses we’re considering, is it really necessary?
Thoughts?
Tony
—————
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
<mailto:tprinte...@twitter.com>
On May 14, 2018 at 12:40:28 PM, Tony Printezis (tprinte...@twitter.com
<mailto:tprinte...@twitter.com>) wrote:
Peter,
In my proposal, you can register the exit hook in the ThreadLocal
c’tor, so it’s almost as nice as Alan’s in that respect (and it
doesn't require an extra field per ThreadLocal plus two extra fields
per JdkEntry). :-)
But, I do like the addition of the JdkEntry list to avoid
unnecessarily iterating over all the map entries (which was my main
concern with Alan’s original webrev). I’ll be totally happy with a
version of this.
Tony
—————
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
<mailto:tprinte...@twitter.com>
On May 12, 2018 at 6:44:08 AM, Peter Levart (peter.lev...@gmail.com
<mailto:peter.lev...@gmail.com>) wrote:
Hi,
On 05/11/18 16:13, Alan Bateman wrote:
On 08/05/2018 16:07, Tony Printezis wrote:
Hi all,
Following the discussion on this a few weeks ago, here’s the first
version
of the change:
http://cr.openjdk.java.net/~tonyp/8202788/webrev.0/
I think the consensus was that it’d be easier if the exit hooks
were only
available within java.base. Is it enough that I added the
functionality to
the jdk.internal.misc package? (And is jdk.internal.misc the best
place for
this?)
I’ll also add a test for the new functionality. But let’s first
come up
with an approach that everyone is happy with. :-)
Peter's approach in early April was clean (and we should come to
the getIfPresent discussion) but it adds a field to Thread for the
callback list. If I read your approach correctly, you are avoiding
that by maintaining an array of hooks in ThreadLocalExitHooks.
Another approach to try is a java.base-internal ThreadLocal that
defines a method to be invoked when a thread terminates. Something
like the following:
http://cr.openjdk.java.net/~alanb/8202788/webrev/index.html
-Alan
From the API perspective, Alan's suggestion is the most attractive
one as it puts the method to where it belongs - into the ThreadLocal
instance. But the implementation could be improved a bit. I don't
like the necessity to iterate over all ThreadLocal(s) to filter out
JdkThreadLocal(s). There might be a situation where there's plenty
of ThreadLocal(s) registered per exiting thread which would produce
a spike in CPU processing at thread exit.
The way to avoid this would be to maintain a separate linked list of
entries that contains just those with JdkThreadLocal(s). Like in
this modification of Alan's patch, for example:
http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.01/
(Only ThreadLocal class is modified from Alan's patch)
What do you think?
Regards, Peter
