RFR (S): 8235247: WorkerDataArray leaks C heap memory for associated work items
leo.korinth at oracle.com
Wed Dec 4 13:11:03 UTC 2019
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.
>>> manual testing using NMT; i did not observe other leaks given the
>>> example program attached to the CR.
>> ~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.
More information about the hotspot-gc-dev