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

[Tony Printezis]

Hi Peter,

Thanks for taking a look at the new webrev!

Initially, I think we’re expecting two uses of JdkThreadLocal: one in
sun.nio.ch.Utils, shown on my webrev and my original motivation for working
on this, and one in sun.nio.fs.NativeBuffers, as shown on Alan’s webrev
(I’m not familiar with that part of the code at all; I assume it’s
addressing a similar issue to sun.nio.ch.Utils).

When I originally brought up this issue, Alan said that he's only expecting
2-3 uses (literally) inside java.base, so I did the implementation
accordingly and tried to keep it as simple as possible. We could maybe look
at other uses of ThreadLocal inside java.base to get a better sense of how
many more will benefit from a thread termination callback?

Response to your comments:

I can definitely add javadoc to explain the limitations of the
implementation. It takes me a long time to write coherent javadoc /
comments, so I wanted to make sure we have the right approach first before
I did that. :-)

Re: sanity tests in add(): I was being paranoid but, sure, I can remove
them.

Re: Entry objects: You’re absolutely right. I did it this way to make it
easier to extend WeakReference if needed. It also keeps the code a bit less
verbose. I can use JdkThreadLocal<T> directly.

Tony

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


On May 17, 2018 at 4:39:20 PM, Peter Levart (peter.levart at gmail.com) 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/

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 | tprintezis at twitter.com


On May 14, 2018 at 12:40:28 PM, Tony Printezis (tprintezis at 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 | tprintezis at twitter.com


On May 12, 2018 at 6:44:08 AM, Peter Levart (peter.levart at 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