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

[Lindenmaier, Goetz]

Thanks Vladimir!

Best regards,
 Goetz.

> -----Original Message-----
> From: Vladimir Kozlov <vladimir.kozlov at oracle.com>
> Sent: Friday, December 20, 2019 10:15 PM
> 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'.
> 
> 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.
> >>>>>>>>>