Hi David, ThreadLocals getting cleared is not enough. The threadLocals field is cleared by Thread::exit: private void exit() { ... threadLocals = null; ... } but this doesn’t really do anything. The GC has to run, find the direct buffer in the ThreadLocal unreachable, and queue its Cleaner for processing. I’m looking for a hook to explicitly reclaim the direct buffer when Thread::exit is called. Tony ————— Tony Printezis | @TonyPrintezis | tprintezis@twitter.com On April 5, 2018 at 6:07:26 PM, David Holmes (david.holmes@oracle.com) wrote: Hi Tony, On 6/04/2018 7:45 AM, Tony Printezis wrote:

Hi all,

We recently hit another interesting issue with the NIO thread-local DirectByteBuffer cache. One of our services seems to create and drop

If it's in a ThreadLocal then aren't you just asking for thread-locals to be cleared at thread exit? Cheers, David

threads at regular intervals. I assume this is due to a thread pool dynamically resizing itself.

Let's say a thread starts, lives long enough for its Thread object to be promoted to the old gen (along with its cached direct buffer), then exits. This will result in its cached direct buffer(s) being unreachable in the old gen and will only be reclaimed after the next full GC / concurrent GC cycle.

Interestingly, the service in question does concurrent GC cycles really infrequently (one every few days) as it has a really low promotion rate. This results in the JVM's total direct size constantly increasing (which is effectively a native buffer leak).

Has anyone come across this issue before?

There are a few obvious ways to mitigate this, e.g., cause a Full GC / concurrent GC cycle at regular intervals. However, the best solution IMHO is to explicitly free any direct buffers that are still in the cache when a thread exits.

I'll be happy to implement this and test it internally at Twitter, if it's not on anyone else's radar. However, to do what I'm proposing I need some sort of thread exit hook. Unfortunately, there doesn't seem to be one.

Would proposing to introduce thread exit hooks be reasonable? Or is everyone going to freak out? :-) The hooks can be either per-Thread or even per-ThreadLocal. And it's OK if the hooks can only be used within java.base.

FWIW, I did a simple prototype of this (I call the hooks from Thread::exit) and it seems to work as expected.

Any thoughts / feedback on this will be very appreciated.

Thanks,

Tony

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

Hi David, Thanks for your thoughts. Please see inline. ————— Tony Printezis | @TonyPrintezis | tprintezis@twitter.com On April 5, 2018 at 6:24:11 PM, David Lloyd (david.lloyd@redhat.com) wrote: On Thu, Apr 5, 2018 at 4:45 PM, Tony Printezis <tprintezis@twitter.com> wrote:

Would proposing to introduce thread exit hooks be reasonable? Or is everyone going to freak out? :-) The hooks can be either per-Thread or even per-ThreadLocal. And it's OK if the hooks can only be used within java.base.

User-accessible thread exit hooks would be nice, from a user perspective. From a JDK perspective - it adds new opportunities for user code to jam things up, so it's a tradeoff. …just like finalizers. :-) Yeah, I get that. Which is why I was a bit reluctant to bring it up. Which is why I proposed to only make it available within java.base? ThreadLocal clearing Could you clarify what you mean by ThreadLocal clearing? on thread exit would have been nice back in the beginning, but now I think it would be a fairly substantial behavior change. Adding a new exit() method on ThreadLocal would be better (but not perfect) compatibility-wise, and see prior note about users jamming things up... I like the suggestion to add an overridable exit() method to ThreadLocal. If you want to avoid calling user code by Thread::exit, would adding ThreadLocals (which are tagged appropriately) to a queue for later processing a better approach (similar to the mechanism used for References / ReferencesQueues)? The user can of course create a memory leak by not polling the queue frequently enough. But, that’s also the case for References. And at least user code cannot block Thread::exit. I think that at a minimum, explicitly releasing thread-local NIO direct buffers on thread exit (without introducing a user facing API) is probably safe. I’m also pretty sure it’s safe. I should have mentioned that the thread-local direct buffers are actually explicitly freed when they are not put back in the cache. So, it should be safe to also explicitly free any that are in the cache when the thread exits. Some kind of user-accessible hook would be nice, but I can't imagine it would take anything less than a massive discussion to get there. I’d like to avoid a massive discussion, which is why I proposed to only make it available within java.base. Tony -- - DML

————— Tony Printezis | @TonyPrintezis | tprintezis@twitter.com On April 6, 2018 at 12:16:10 PM, David Lloyd (david.lloyd@redhat.com) wrote: On Fri, Apr 6, 2018 at 8:57 AM, Tony Printezis <tprintezis@twitter.com> wrote:

...

ThreadLocal clearing

Could you clarify what you mean by ThreadLocal clearing?

I mean calling ThreadLocal#remove(). I see. So, anyone who subclasses ThreadLocal can override remove(). And if remove() is called by Thread::exit, it can be used as an exit hook. (David Holmes, if this is what you meant in your e-mail: apologies; I misunderstood.)

I like the suggestion to add an overridable exit() method to ThreadLocal. If you want to avoid calling user code by Thread::exit, would adding ThreadLocals (which are tagged appropriately) to a queue for later processing a better approach (similar to the mechanism used for References / ReferencesQueues)? The user can of course create a memory leak by not polling the queue frequently enough. But, that’s also the case for References. And at least user code cannot block Thread::exit.

It's more complexity, and at some point you have to ask: is it better to block thread A or thread B? At least blocking thread A is somewhat expected. I agree re: it’d add complexity. #simplify :-) Tony -- - DML

On 04/09/18 08:25, David Holmes wrote:

On 7/04/2018 3:11 AM, Tony Printezis wrote:

...

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

On April 6, 2018 at 12:16:10 PM, David Lloyd (david.lloyd@redhat.com) wrote:

On Fri, Apr 6, 2018 at 8:57 AM, Tony Printezis <tprintezis@twitter.com> wrote:

...

...

ThreadLocal clearing

Could you clarify what you mean by ThreadLocal clearing?

I mean calling ThreadLocal#remove().

I see. So, anyone who subclasses ThreadLocal can override remove(). And if remove() is called by Thread::exit, it can be used as an exit hook. (David Holmes, if this is what you meant in your e-mail: apologies; I misunderstood.)

Absolutely! ... Well kind of ... okay actually no, I wasn't thinking at that level of detail yet. ;-)

Cheers, David H.

There's no need in calling ThreadLocal.remove() at thread exit as Thread.exit() method already does that (sort of):     private void exit() {         ...         threadLocals = null;         inheritableThreadLocals = null;         ... After thread exits, ThreadLocal values associated with it are no longer reachable from its Thread object. The problem Tony faces is that by the time this happens, direct ByteBuffer's that were cached using such ThreadLocal value, are already moved to old GC generation, waiting for full GC to release them together with direct memory they are holding. So what is needed is an internal call-back registration API Alan is proposing and (maybe just internal) probing method for ThreadLocal that return(s) the associated value only when it has already been initialized. Regards, Peter

...

...

I like the suggestion to add an overridable exit() method to ThreadLocal. If you want to avoid calling user code by Thread::exit, would adding ThreadLocals (which are tagged appropriately) to a queue for later processing a better approach (similar to the mechanism used for References / ReferencesQueues)? The user can of course create a memory leak by not polling the queue frequently enough. But, that’s also the case for References. And at least user code cannot block Thread::exit.

It's more complexity, and at some point you have to ask: is it better to block thread A or thread B? At least blocking thread A is somewhat expected.

I agree re: it’d add complexity. #simplify :-)

Tony

On 04/09/18 10:33, David Holmes wrote:

...

After thread exits, ThreadLocal values associated with it are no longer reachable from its Thread object.

The problem Tony faces is that by the time this happens, direct ByteBuffer's that were cached using such ThreadLocal value, are already moved to old GC generation, waiting for full GC to release them together with direct memory they are holding.

Right. So the suggestion was to call ThreadLocal.remove() instead (as well as?) so that you could define a custom ThreadLocal class for the buffers that override remove() to actually release the buffer directly.

David

Ah, of course. But the concern is that custom code at thread-exit is not desirable. Alan is propagating private internal API. Here's a prototype: http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.00/ Would something like this be acceptable? Regards, Peter

On 09/04/2018 11:26, Peter Levart wrote:

If adding a field to Thread class is a concern, I can modify this to use a special ThreadLocal instance to keep registered callback(s) per thread.

Thanks for the prototype, that is along the lines of what I was thinking. I also think it would be good to prototype is as a thread local so that there isn't another Thread field. Another thought: if threads are guaranteed to free the cached buffers then we may be able to create the direct buffer without an associated cleaner. -Alan

On 04/06/2018 10:02 AM, Alan Bateman wrote:

On 05/04/2018 22:45, Tony Printezis wrote:

...

Hi all,

We recently hit another interesting issue with the NIO thread-local DirectByteBuffer cache. One of our services seems to create and drop threads at regular intervals. I assume this is due to a thread pool dynamically resizing itself.

Let's say a thread starts, lives long enough for its Thread object to be promoted to the old gen (along with its cached direct buffer), then exits. This will result in its cached direct buffer(s) being unreachable in the old gen and will only be reclaimed after the next full GC / concurrent GC cycle.

Right, if a short lived thread does I/O with a buffer backed by an array in the heap then any direct buffers in its thread local cache won't be freed until there is a GC and reference processing. It's something that I've prototyped a few times and always leaned towards not exposing anything in the API, instead just hooking into the thread exit to clear the buffer cache. One thing to watch out for is that the buffer cache may not exist (as the thread didn't do any I/O with heap buffers) so you'll end up creating (an empty) buffer cache at thread exit. That is benign of course but still unsettling to have additional code executing at this time.

-Alan

An internal method, let's say ThreadLocal.probe(), that would return thread-local value only if it has already been initialized, would be helpfull. Maybe it could be exposed as new public API too. I remember that I needed it in the past:     public T probe() {         Thread t = Thread.currentThread();         ThreadLocalMap map = getMap(t);         if (map != null) {             ThreadLocalMap.Entry e = map.getEntry(this);             if (e != null) {                 @SuppressWarnings("unchecked")                 T result = (T)e.value;                 return result;             }         }         return null;     } Regards, Peter

Hi Alan, Calling sun.nio directly from java.lang seemed a bit dodgy to me, which is why I proposed some type of exit hook (maybe I overthought this?). But that’d be OK, could make this change a lot simpler. :-) And, yes, I came across the issue of not being able to query whether a ThreadLocal exists on a Thread when I implemented my prototype. Which is why I think introducing an exit hook on ThreadLocal, instead of Thread, is probably the better approach (it will only be called if the ThreadLocal exists). Tony ————— Tony Printezis | @TonyPrintezis | tprintezis@twitter.com On April 6, 2018 at 4:02:56 AM, Alan Bateman (alan.bateman@oracle.com) wrote: On 05/04/2018 22:45, Tony Printezis wrote:

Hi all,

We recently hit another interesting issue with the NIO thread-local DirectByteBuffer cache. One of our services seems to create and drop threads at regular intervals. I assume this is due to a thread pool dynamically resizing itself.

Let's say a thread starts, lives long enough for its Thread object to be promoted to the old gen (along with its cached direct buffer), then exits. This will result in its cached direct buffer(s) being unreachable in the old gen and will only be reclaimed after the next full GC / concurrent GC cycle.

Right, if a short lived thread does I/O with a buffer backed by an array in the heap then any direct buffers in its thread local cache won't be freed until there is a GC and reference processing. It's something that I've prototyped a few times and always leaned towards not exposing anything in the API, instead just hooking into the thread exit to clear the buffer cache. One thing to watch out for is that the buffer cache may not exist (as the thread didn't do any I/O with heap buffers) so you'll end up creating (an empty) buffer cache at thread exit. That is benign of course but still unsettling to have additional code executing at this time. -Alan

Hi Alan, Ah, I hadn’t realized that there’s already some tight coupling between Thread and nio. OK, I’ll just call into sun.nio directly and see what the reviewers say. :-) Is there a CR for this already? Or should I create one? Tony ————— Tony Printezis | @TonyPrintezis | tprintezis@twitter.com On April 6, 2018 at 12:45:29 PM, Alan Bateman (alan.bateman@oracle.com) wrote: On 06/04/2018 15:08, Tony Printezis wrote: Hi Alan, Calling sun.nio directly from java.lang seemed a bit dodgy to me, which is why I proposed some type of exit hook (maybe I overthought this?). But that’d be OK, could make this change a lot simpler. :-) And, yes, I came across the issue of not being able to query whether a ThreadLocal exists on a Thread when I implemented my prototype. Which is why I think introducing an exit hook on ThreadLocal, instead of Thread, is probably the better approach (it will only be called if the ThreadLocal exists). java.lang.Thread is already tightly connected to blocking I/O operations (Thread::interrupt for example). In any case, the concern with exposing exit hooks in the API is that it means running arbitrary code when exiting, something that would need a lot of consideration before going there. -Alan

On Fri, Apr 6, 2018 at 1:02 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:

On 05/04/2018 22:45, Tony Printezis wrote:

...

We recently hit another interesting issue with the NIO thread-local DirectByteBuffer cache. One of our services seems to create and drop threads at regular intervals. I assume this is due to a thread pool dynamically resizing itself.

ThreadPoolExecutor will replace any worker thread that had a throwing task, but that only happens on execute, (not submit, because of additional wrapping by a Future). Perhaps a mistake that is too late to fix. Keep your own worker threads from dying by making sure that all tasks are Future-wrapped.

On 21/06/2018 21:13, Florian Weimer wrote:

* Tony Printezis:

...

There are a few obvious ways to mitigate this, e.g., cause a Full GC / concurrent GC cycle at regular intervals. However, the best solution IMHO is to explicitly free any direct buffers that are still in the cache when a thread exits. Why is this safe? Couldn't these direct byte buffers be used with a custom channel that leaks them to another thread? The temporary direct buffer mechanism is internal to java.base so it should never be used with custom channel implementations. There may be some pre-existing issues with the FileChannel transferXXX methods when called with an untrusted source/sink that will need to be audited but this is not something that I can discuss on this mailing list.

-Alan