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

[Lindenmaier, Goetz]

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 :)?

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