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

[David Holmes]

On 19/12/2019 9:35 pm, Lindenmaier, Goetz wrote:
> Hi,
> 
> yes, it is confusing that parts are on the arena, other parts
> are allocated in the C-heap.
> But usages which allocate the stringStream with new() are
> rare, usually it's allocated on the stack making all this
> more simple.  And the previous design was even more
> error-prone.
> Also, the whole way to print the inlining information
> is quite complex, with strange usage of the copy constructor
> of PrintInliningBuffer ... which reaches into GrowableArray
> which should have a constructor that does not use the
> copy constructor to initialize the elements ...
> 
> I do not intend to change stringStream in this change.
> So can I consider this reviewed from your side? Or at
> least that there is no veto :)?

Sorry I was trying to convey this is Reviewed, but I do think this needs 
further work in the future.

Thanks,
David

> Thanks and best regards,
>    Goetz.
> 
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Donnerstag, 19. Dezember 2019 11:39
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 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 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.
>>>>>>