RFR: 8154715: Missing destructor and/or TLS clearing calls for terminating threads

[Stefan Karlsson]

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
>>