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

[Vladimir Kozlov]

Please, file RFE for refactoring stringStream.

Yes, the fix can go into JDK 14.

But before that, I see the same pattern used 3 times in compile.cpp:

+    if (_print_inlining_stream != NULL) _print_inlining_stream->~stringStream();
      _print_inlining_stream = <something>;

Can you use one function for that? Also our coding style requires to put body of 'if' on separate line and use {}.

thanks,
Vladimir

On 12/19/19 4:52 AM, David Holmes wrote:
> 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.
>>>>>>>