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

[Leo Korinth]

On 04/12/2019 13:26, Thomas Schatzl wrote:
> 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.

I am still okay with your change, and I agree with shipping it "as is".

Eventually I would really appreciate a clean-up or even rewrite. Every 
time I look into the "half recursive" WorkerDataArray I get extremely 
confused. IMO WorkerDataArray should be /two/ separate types, big parts 
of the API is only applicable to the "aggregated" arrays. I also agree 
with Kim that ownership is unclear; maybe (in the future) make 
link_thread_work_items into allocate_thread_work_items to have 
allocation and deletion of sub-arrays in the same class. A tracking CR 
for cleaning up the deletes Kim mentioned would be good.

Thanks,
Leo

> Thanks,
>    Thomas