David, Please see inline.
————— Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On May 8, 2018 at 11:44:11 AM, David Lloyd (david.ll...@redhat.com) wrote: I'm not a reviewer, but I would ask: how sure are we that it's OK to use lambdas from here? Is there a chance that the magic bootstrap stuff won't yet be initialized at the time when an early thread could exit for whatever reason? I’m not aware that lambdas cannot be used at particular points in the JVM’s execution. Is that really a concern? (FWIW, the code I posted compiles and runs fine.) Anyway I think using a lambda with forEach is probably overkill in atThreadExit. A simple for loop would suffice IMO. That’s not straightforward, though. Note that the iteration is done by the ThreadLocal class, not the ThreadLocalExitHooks class. So, a for-loop won’t work, unless I expose the array to the ThreadLocal class, which I’d rather not do. I could use an Iterator. But using forEach() is a lot nicer (IMHO of course). Also, I would be quite surprised if there wasn't a way to get a system property from system code without having to use doPrivileged; that might bear some researching. I just copied how the Utils class reads the jdk.nio.maxCachedBufferSize property (see the existing getMaxCachedBufferSize() method). Could this all be simplified to drop the necessity for passing around the thread local map and thread local instances? The sun.nio.ch.Util has static access to its ThreadLocal, so really all it needs is to be able to give a Runnable to run at thread exit and it can take care of the rest. Not quite sure what you mean by “passing around the thread local map and thread local instances”. The map is only passed from Thread to ThreadLocal, so the latter can call exit hooks (if any). So, the map doesn’t escape the Thread / ThreadLocal classes. The thread local instance is only used to associate that instance with an exit hook in the ThreadLocalExitHooks class. It’d be nice if I could pass a Runnable to be called at thread exit. However, how do I do that? Will there be a method on ThreadLocal to set this Runnable. If that’s the case, can we arrange for a method on ThreadLocal to only be used within java.base module (remember: we don’t want this functionality to be used outside java.base)? Additionally, if it’s just a Runnable, how does it get the thread’s ThreadLocal value when it runs? If the ThreadLocal is not set for that thread, get() will create it, which will be a waste of an allocation. It could be a simple as keeping an ArrayList of Runnables on Thread or in another ThreadLocal. But this requires at least a couple of objects per Thread. What I implemented has a single entry for each ThreadLocal, irrespective of how many threads set it (so a lot less overhead). Tony On Tue, May 8, 2018 at 10:07 AM, Tony Printezis <tprinte...@twitter.com> 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. :-) > > One additional question: I put the new functionality conditional on a > property (jdk.nio.freeBuffersAtThreadExit) and it’s off by default. Should > I make it on by default? Or just not add the property all-together? > > Tony > > > ————— > Tony Printezis | @TonyPrintezis | tprinte...@twitter.com -- - DML
