RFR (S): 8235247: WorkerDataArray leaks C heap memory for associated work items

[Thomas Schatzl]

Hi Kim,

On 03.12.19 15:47, Kim Barrett wrote:
>> On Dec 3, 2019, at 8:02 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>>
>> Hi all,
>>
>>   can I have reviews for this fix to a small memory leak, leaking a few 100 bytes per GC?
>>
>> WorkerDataArray does not delete linked thread work items, so code that temporarily creates and uses them leaks.
>>
>> Currently this is only a variant of WeakProcessor::weak_oops_do() that seems to be only used by G1.
>>
>> Thanks go to M. Vidstedt for finding and reporting the issue.
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8235247
>> Webrev:
>> http://cr.openjdk.java.net/~tschatzl/8235247/webrev/
>> Testing:
>> manual testing using NMT; i did not observe other leaks given the example program attached to the CR.
>>
>> Thanks,
>>   Thomas
> 
> ~WeakProcessorPhaseTimes has delete expressions to deal with this. The
> problem is that the arrays whose elements are being deleted
> (_worker_dead_items and _worker_total_items) don't contain the
> expected WorkerDataArray objects. The constructor just calls
> link_thread_work_items on the new WorkerDataArrays directly, and does
> not record the new WorkerDataArrays in in those array members.
> 
> I was expecting this change to introduce double-frees in
> G1GCPhaseTimes, but it seems there is no destructor or call to the
> destructor for that class.
> 
> I think the right fix for the leak is to fix the
> WeakProcessorPhaseTimes constructor, rather than making
> link_thread_work_items (implicitly) transfer ownership of the argument
> to "this".
> 

   we talked about this internally and the results were that due to bad 
WorkerDataArray API it is unclear who should be the owner of these 
worker data items; we both agreed on making the "main" WorkerDataArray 
owner in the future, as there is no use case to share the item arrays. I 
will look into improving the API to make the usage less confusing asap 
(including cleaning up code that assumes otherwise), but for now as a 
point fix keep the change as is.

If nobody objects I will push this change later today.

Thanks,
   Thomas