RFR(M): 8235998: [c2] Memory leaks during tracing after '8224193: stringStream should not use Resouce Area'.

[David Holmes]

On 19/12/2019 7:27 pm, Lindenmaier, Goetz wrote:
> Hi David, Vladimir,
> 
> stringStream is a ResourceObj, thus it lives on an arena.
> This is uncritical, as it does not resize.
> 8224193 only changed the allocation of the internal char*,
> which always caused problems with resizing under
> ResourceMarks that were not placed for the string but to
> free other memory.
> Thus stringStream must not be deallocated, and
> also there was no mem leak before that change.
> But we need to call the destructor to free the char*.

I think we have a confusing mix of arena and C_heap usage with 
stringStream. Not clear to me why stringStream remains a resourceObj 
now? In many cases the stringStream is just local on the stack. In other 
cases if it is new'd then it should be C-heap same as the array and then 
you could delete it too.

What you have may suffice to initially address the leak but I think this 
whole thing needs revisiting.

Thanks,
David

> 
> Best regards,
>    Goetz.
> 
>> -----Original Message-----
>> From: hotspot-runtime-dev <hotspot-runtime-dev-bounces at openjdk.java.net>
>> On Behalf Of David Holmes
>> Sent: Mittwoch, 18. Dezember 2019 04:33
>> To: Vladimir Kozlov <vladimir.kozlov at oracle.com>; hotspot-compiler-
>> dev at openjdk.java.net
>> Cc: hotspot-runtime-dev at openjdk.java.net runtime <hotspot-runtime-
>> dev at openjdk.java.net>
>> Subject: Re: RFR(M): 8235998: [c2] Memory leaks during tracing after
>> '8224193: stringStream should not use Resouce Area'.
>>
>> On 18/12/2019 12:40 pm, Vladimir Kozlov wrote:
>>> CCing to Runtime group.
>>>
>>> For me the use of `_print_inlining_stream->~stringStream()` is not obvious.
>>> I would definitively miss to do that if I use stringStreams in some new
>>> code.
>>
>> But that is not a problem added by this changeset, the problem is that
>> we're not deallocating these stringStreams even though we should be.  If
>> you use a stringStream in new code you have to manage its lifecycle.
>>
>> That said why is this:
>>
>>    if (_print_inlining_stream != NULL)
>> _print_inlining_stream->~stringStream();
>>
>> not just:
>>
>> delete _print_inlining_stream;
>>
>> ?
>>
>> Ditto for src/hotspot/share/opto/loopPredicate.cpp why are we explicitly
>> calling the destructor rather than calling delete?
>>
>> Cheers,
>> David
>>
>>> May be someone can suggest some C++ trick to do that automatically.
>>> Thanks,
>>> Vladimir
>>>
>>> On 12/16/19 6:34 AM, Lindenmaier, Goetz wrote:
>>>> Hi,
>>>>
>>>> I'm resending this with fixed bugId ...
>>>> Sorry!
>>>>
>>>> Best regards,
>>>>     Goetz
>>>>
>>>>> Hi,
>>>>>
>>>>> PrintInlining and TraceLoopPredicate allocate stringStreams with new and
>>>>> relied on the fact that all memory used is on the ResourceArea cleaned
>>>>> after the compilation.
>>>>>
>>>>> Since 8224193 the char* of the stringStream is malloced and thus
>>>>> must be freed. No doing so manifests a memory leak.
>>>>> This is only relevant if the corresponding tracing is active.
>>>>>
>>>>> To fix TraceLoopPredicate I added the destructor call
>>>>> Fixing PrintInlining is a bit more complicated, as it uses several
>>>>> stringStreams. A row of them is in a GrowableArray which must
>>>>> be walked to free all of them.
>>>>> As the GrowableArray is on an arena no destructor is called for it.
>>>>>
>>>>> I also changed some as_string() calls to base() calls which reduced
>>>>> memory need of the traces, and added a comment explaining the
>>>>> constructor of GrowableArray that calls the copyconstructor for its
>>>>> elements.
>>>>>
>>>>> Please review:
>>>>> http://cr.openjdk.java.net/~goetz/wr19/8235998-
>> c2_tracing_mem_leak/01/
>>>>>
>>>>> Best regards,
>>>>>     Goetz.
>>>>