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/
Or the Alan's with my mods:
http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.02/
Regards, Peter
On 05/30/18 22:46, Tony Printezis wrote:
Hi all,
Any more thoughts on this? (with apologies for the ping)
Tony
—————
Tony Printezis | @TonyPrintezis | [email protected]
<mailto:[email protected]>
On May 18, 2018 at 3:58:57 PM, Tony Printezis ([email protected]
<mailto:[email protected]>) wrote:
Hi again,
Stylistically, I strongly prefer this version over the previous one
(the one with the doubly-linked list of JdkThreadLocal entries) you
had posted. This one is a lot cleaner.
A few suggestions:
* I’m not crazy about the fact that the users have to override
calculateInitialValue() instead of initialValue() as it will be a bit
error-prone. If they accidentally override initialValue() then the
per-thread registering is not going to happen. Maybe I’m overthinking
this. One way to get round this is for each JdkThreadLocal instance
to delegate calls to get(), set(), and remove() to a private
ThreadLocal instance, which can in turn delegate initialValue() to
the outer instance. (Hope this makes sense?) I did a quick prototype
of this and it seems to work, but I didn’t heavily test it.
* I would prefer if the method users override actually took the
thread-local value as a parameter, i.e., protected void
threadTerminated(T value). This is very easy to add:
protected void threadTerminated(T value) {
}
private void threadTerminated() {
threadTerminated(get());
}
and I think it will be easier to use, as most uses will probably need
to do get() anyway.
Tony
—————
Tony Printezis | @TonyPrintezis | [email protected]
<mailto:[email protected]>
On May 18, 2018 at 4:23:19 AM, Peter Levart ([email protected]
<mailto:[email protected]>) wrote:
It's good to have alternative implementations on the table. Here's
another variant that is space neutral for threads that don't use
JdkThreadLocal, but also scales to many JdkThreadLocal instances:
http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.02/
Now we only need an arbiter to decide which one! :-)
Regards, Peter
On 05/17/18 22:39, Peter Levart wrote:
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 | [email protected]
<mailto:[email protected]>
On May 14, 2018 at 12:40:28 PM, Tony Printezis
([email protected] <mailto:[email protected]>) 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 | [email protected]
<mailto:[email protected]>
On May 12, 2018 at 6:44:08 AM, Peter Levart
([email protected] <mailto:[email protected]>) 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
