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

Wed Jun 6 20:38:06 UTC 2018

Hi Peter,

Thanks for the updated webrev! Please see inline.

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

On June 6, 2018 at 2:55:51 PM, Peter Levart (peter.levart at gmail.com) 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

#ThumbsUp

- instead of overriding initialValue(), ThreadLocal.setInitialValue() calls
TerminatingThreadLocal.register() at the right moment

Thanks. I much prefer not having to introduce calculateInitialValue().

One extra suggestion: Given you introduced a call to TerminatingThreadLocal
from ThreadLocal, would it make sense to maybe do the same for set() and
remove() (you’d have to add an appropriate check in unregister) and not
override them in TerminatingTreadLocal? I totally get why you might not
want to do that (an extra check when a ThreadLocal is not the Terminating
one). I’m OK either way.

- TerminatingThreadLocal.threadTerminated() now takes a (T value) parameter

#ThumbsUp

- TerminatingThreadLocal.REGISTRY uses IdentityHashSet instead of HashSet
(if .equals()/.hashCode() are overriden in some TerminatingThreadLocal
subclass)

#ThumbsUp

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.

That’s a very good point.

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.

Something like:

if (tl.isPresent()) {

   T val = t.get();

   ….

}

will do two look-ups if the value exists. However, that’s better than
allocating unnecessarily. So, I’ll take isPresent() over not having a way
to check whether a value exists.

Thumbs up from me.

Tony

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

On June 6, 2018 at 11:12:44 AM, Peter Levart (peter.levart at 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 | tprintezis at twitter.com

On June 6, 2018 at 9:38:05 AM, Alan Bateman (alan.bateman at 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/
>
> Or the Alan's with my mods:
>
> http://cr.openjdk.java.net/~plevart/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