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

Fri Dec 20 21:14:54 UTC 2019

On 12/20/19 2:12 AM, Lindenmaier, Goetz wrote:
> Hi Vladimir,
> 
> I refactored it a bit, see new webrev:
> http://cr.openjdk.java.net/~goetz/wr19/8235998-c2_tracing_mem_leak/02/

Looks good to me.

> 
> Also, I filed
> 8236414: stringStream allocates on ResourceArea and C-heap
> https://bugs.openjdk.java.net/browse/JDK-8236414
> ... but I'm not sure how to solve it, as stringStream inherits
> this capability, and having the char* on the ResourceArea
> as before is not good, either.
> Anyways, there are very few places where new is used
> with the stringStream, and the PrintInlining implementation
> is the only one where it's problematic. Maybe it would be
> better to simplify PrintInlining.

Yes, simplifying PrintInlining is also option.

Thanks,
Vladimir

> 
> Best regards,
>    Goetz.
> 
>> -----Original Message-----
>> From: Vladimir Kozlov <vladimir.kozlov at oracle.com>
>> Sent: Donnerstag, 19. Dezember 2019 19:07
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.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'.
>>
>> 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.
>>>>>>>>>