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

Wed May 30 21:16:42 UTC 2018

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 | tprintezis at twitter.com 
> <mailto:tprintezis at twitter.com>
>
>
> On May 18, 2018 at 3:58:57 PM, Tony Printezis (tprintezis at twitter.com 
> <mailto:tprintezis at twitter.com>) 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 | tprintezis at twitter.com 
>> <mailto:tprintezis at twitter.com>
>>
>>
>> On May 18, 2018 at 4:23:19 AM, Peter Levart (peter.levart at gmail.com 
>> <mailto:peter.levart at gmail.com>) 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 | tprintezis at twitter.com 
>>>>> <mailto:tprintezis at twitter.com>
>>>>>
>>>>>
>>>>> On May 14, 2018 at 12:40:28 PM, Tony Printezis 
>>>>> (tprintezis at twitter.com <mailto: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 
>>>>>> <mailto:tprintezis at twitter.com>
>>>>>>
>>>>>>
>>>>>> On May 12, 2018 at 6:44:08 AM, Peter Levart 
>>>>>> (peter.levart at gmail.com <mailto: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
>>>>>>>
>>>>
>>>