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

[Lindenmaier, Goetz]

Hi Vladimir, 

I refactored it a bit, see new webrev:
http://cr.openjdk.java.net/~goetz/wr19/8235998-c2_tracing_mem_leak/02/

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.

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.
> >>>>>>>