Hi David,
On 10/05/16 12:17, David Holmes wrote:
Hi Stefan,
Thanks for looking at this again.
On 10/05/2016 6:24 PM, Stefan Karlsson wrote:
Hi David,
On 10/05/16 02:33, David Holmes wrote:
Okay here is version 2:
http://cr.openjdk.java.net/~dholmes/8154715/webrev.v2/
Lots of cosmetic changes but only a couple of functional ones:
- After thread->run() returns we clear the TLS by calling
clear_thread_current(), but only for threads where it has not already
been cleared - as those threads may already have been deleted so we
can't dereference 'thread'
- No asynchronous thread deletion is permitted, and we avoid races
with VM termination. This means the VMThread no longer gets deleted -
that should not be an issue as many threads do not get deleted when
the VM terminates. I added destructors for the VMThread and
WatcherThread so anyone introducing their deletion is informed by a
guarantee(false)
This makes the model easier to understand, IMHO. Either you delete the
thread from the run() method, or you don't delete it at all.
I's there a way to determine how much memory we leak by not deleting the
memory owned by the VMThread instance? I'm a bit worried that the
VMThread might use more resources than the other threads we don't
delete.
There is nothing at all special about the VMThread instance, it is
just a NamedThread with nothing being referenced by instance fields
beyond the name. Any runtime resources are released as the thread
terminates - and nothing special there either AFAICS.
I was more thinking about the contents of the following memory:
Thread::~Thread() {
ObjectSynchronizer::omFlush(this);
[...]
delete resource_area();
[...]
delete handle_area();
delete metadata_handles();
The amount of memory that we don't hand back is about 1500 bytes for the
VMThread. Most comes from the resource_area(), which is initialized to
(init_size = 1*K - slack) in the Thread constructor.
The amount of memory doesn't seem to be out-of-the ordinary, so leaking
the VMThread doesn't seem to be worse than leaking the other threads, as
you said.
The patch looks good to me, but you probably want to get a Review by
someone well-versed in the HotSpot threading system.
Thanks,
StefanK
Thanks,
David
Thanks,
StefanK
Cosmetic changes:
- renamed java_start to thread_native_entry (it is used by all threads
not just "java" ones, so this avoids potential confusion)
- updated os::free_thread to always assume it works on the current
thread (and add assert to verify that)
Thanks,
David
