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

[Thomas Schatzl]

Hi Leo,

On 04.12.19 14:11, Leo Korinth wrote:
> 
> 
> 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:
[...]>>>
>>> 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".

Thanks for your understanding.

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

I already finished cleaning up the issues Kim mentioned and I will file 
an RFE for this cleanup change later and post it. These do not cover 
breaking up things further, but I agree that G1GCPhaseTimes is ripe for 
a good refactoring; another item on the wish list would be to use 
Ticks/Tickspan for storage of the times.
Even more changes like better (and simpler/easier) use with parallel 
tasks/AbstractGangTask, potentially integrating into them directly, have 
been floating around for some time.

There are already RFEs for some of that too - just that there hasn't 
been anyone picking these up in earnest.

Thanks,
   Thomas