RFR(S): 8149557: Resource mark breaks printing to string stream

[Lindenmaier, Goetz]

Hi David, 

thanks for sponsorig!

Best regards,
  Goetz.

> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Freitag, 4. März 2016 09:55
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'Thomas Stüfe'
> <thomas.stuefe at gmail.com>
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(S): 8149557: Resource mark breaks printing to string stream
> 
> Hi Goetz,
> 
> I can sponsor, unless someone else grabs it first - my weekend has
> already started so I won't get to it for a couple of days. :)
> 
> David
> 
> On 4/03/2016 6:37 PM, Lindenmaier, Goetz wrote:
> >
> >
> >> -----Original Message-----
> >> From: David Holmes [mailto:david.holmes at oracle.com]
> >> Sent: Tuesday, March 01, 2016 5:29 AM
> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'Thomas Stüfe'
> >> <thomas.stuefe at gmail.com>
> >> Cc: hotspot-runtime-dev at openjdk.java.net
> >> Subject: Re: RFR(S): 8149557: Resource mark breaks printing to string
> stream
> >>
> >> Hi Goetz,
> >>
> >> On 29/02/2016 11:09 PM, Lindenmaier, Goetz wrote:
> >>> Hi David, Thomas,
> >>>
> >>> you are right, I need to check for NULL. I had the linux
> >>> case in mind.
> >>> New webrev:
> >>> http://cr.openjdk.java.net/~goetz/wr16/8149557-ResMrk/webrev.02/
> >>
> >> I can't help but feel there is a bug in the Resource-area memory
> >> management code here. The problem as I understand it is that we have:
> >>
> >>    void Symbol::print_symbol_on(outputStream* st) const {
> >>       ResourceMark rm;
> >>       st = st ? st : tty;
> >>       st->print("%s", as_quoted_ascii());
> >>     }
> >>
> >> where we need the ResourceMark for the string returned by
> >> as_quoted_ascii. But if st is itself a resource-area object and needs to
> >> re-size in st->print then it does that in the context of the new
> >> ResourceMark, which fails the assert: "stringStream is re-allocated with
> >> a different ResourceMark." That seems to be the bug to me - a
> >> resource-area object that needs resizing should be able to do it in the
> >> context of an outer ResourceMark. :(
> >>
> >> So the workaround is to use a separate ResourceMark to call
> >> as_quoted_ascii and then copy the result to a malloc'd string, which is
> >> used for the st->print (which may realloc in the context of the existing
> >> outer ResourceMark) and is then free()'d. Yuck. :(
> > Yes, this exactly describes the situation.  I wouldn't call it a bug in the
> Resource-area memory
> > management, but an unpleasant implementation detail of stringStream,
> that
> > makes it behave differently from its related classes.
> >
> >>
> >> Reluctantly Reviewed.
> > Thanks!
> >
> > Would you mind sponsoring?
> >
> > Best regards,
> >    Goetz.
> >
> >
> >>
> >> Thanks,
> >> David
> >>
> >>
> >>> Best regards,
> >>>     Goetz.
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>> Sent: Montag, 22. Februar 2016 01:55
> >>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'Thomas Stüfe'
> >>>> <thomas.stuefe at gmail.com>
> >>>> Cc: hotspot-runtime-dev at openjdk.java.net
> >>>> Subject: Re: RFR(S): 8149557: Resource mark breaks printing to string
> >> stream
> >>>>
> >>>> On 19/02/2016 11:05 PM, Lindenmaier, Goetz wrote:
> >>>>> Hi Thomas,
> >>>>>
> >>>>> thanks for looking at this.
> >>>>>
> >>>>>> you should check the return value of os::strdup().
> >>>>> strdup returns null on oom, %s should be ‘null’, and
> >>>>
> >>>> What are you assuming takes care of %s to 'null' conversion? gcc printf
> >>>> may do it but AFAIK it is undefined at the C library level.
> >>>>
> >>>> David
> >>>> -----
> >>>>
> >>>>> free handles null properly, too.  So I think this is fine.
> >>>>>
> >>>>> If I remove the RM, I might break callers. E.g.
> >>>> TestLogTouchedMethods.java.
> >>>>> Also, in that test, all logged method names will accumulate in
> memory.
> >>>>>
> >>>>> Best regards,
> >>>>>      Goetz.
> >>>>>
> >>>>>
> >>>>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> >>>>> Sent: Thursday, February 11, 2016 9:41 AM
> >>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> >>>>> Cc: hotspot-runtime-dev at openjdk.java.net
> >>>>> Subject: Re: RFR(S): 8149557: Resource mark breaks printing to string
> >>>> stream
> >>>>>
> >>>>> Hi Goetz,
> >>>>>
> >>>>> Thank you for fixing this. I see you are restless :-)
> >>>>>
> >>>>> you should check the return value of os::strdup(). The change looks
> fine
> >>>> otherwise, though I wonder whether we could just omit the
> >> ResourceMark
> >>>> and using the one of the caller. At least it is inconsistent, because
> >>>> Symbol::print_utf8_on() does not have a ResourceMark either.
> >>>>>
> >>>>> Kind regards, Thomas
> >>>>>
> >>>>>
> >>>>>
> >>>>> On Thu, Feb 11, 2016 at 9:24 AM, Lindenmaier, Goetz
> >>>> <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>>
> >> wrote:
> >>>>> Hi
> >>>>>
> >>>>> Please review this change. I please need a sponsor.
> >>>>> http://cr.openjdk.java.net/~goetz/wr16/8149557-
> ResMrk/webrev.01/
> >>>>>
> >>>>> If a stringStream is passed to Symbol::print_symbol_on(),
> >>>>> assertion "stringStream is re-allocated with a different
> ResourceMark."
> >> can
> >>>> fire in stringStream::write(const char* s, size_t len).
> >>>>> This can be verified by running TestLogTouchedMethods.java after
> >> adding
> >>>> the patch below to the VM.
> >>>>>
> >>>>> My fix copies the string to locally managed memory before popping
> the
> >>>> ResoreceMark.
> >>>>> A better solution would be to guarantee enough space on the stream
> >>>>> before doing the resource mark, but outputStream has no such
> >>>>> method as 'ensureCapacity' or the like.
> >>>>> Alternatively the ResourceMark could be skipped altogether.
> >>>>> But then the aux memory used remains on the ResourceArea too
> long.
> >>>>> Also, various ResourceMarks have to be added in other places.
> >>>>>
> >>>>> Best regards,
> >>>>>      Goetz.
> >>>>>
> >>>>>
> >>>>> --- a/src/share/vm/runtime/java.cpp     Wed Jan 13 11:33:21 2016
> +0100
> >>>>> +++ b/src/share/vm/runtime/java.cpp     Wed Feb 10 08:56:14 2016
> >> +0100
> >>>>> @@ -340,7 +340,10 @@
> >>>>>       }
> >>>>>
> >>>>>       if (LogTouchedMethods && PrintTouchedMethodsAtExit) {
> >>>>> -    Method::print_touched_methods(tty);
> >>>>> +    ResourceMark rm;
> >>>>> +    stringStream st(16);
> >>>>> +    Method::print_touched_methods(&st);
> >>>>> +    tty->print("%s", st.as_string());
> >>>>>       }
> >>>>>
> >>>>>       if (PrintBiasedLockingStatistics) {
> >>>>>