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

[Thomas Stüfe]

Hi David,

On Tue, Mar 1, 2016 at 5:29 AM, David Holmes <david.holmes at oracle.com>
wrote:

> 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. :(
>
>
I think the way resource areas are designed this is not possible.
ResourceArea is a thread-local memory stack (actually a linked list of
chunks), and ResourceMark just a watermark into this stack. In order to
"allocate in the context of any ResourceMark" on the stack above the call
site, each ResourceMark would have to itself own an expandable arena. I
think this would be more expensive and complicated.

I also think cases like this are rare - where objects keep pointers to
resource area storage and the objects themselves are handed up and down the
stack. The only other case I know was Marcus' ttyLocker replacement message
buffer for UL.


> 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. :(
>
>
Another workaround would have been to get rid of the ResourceMark
altogether, because it is used inconsequently. Symbol class has a number of
print functions, not all of those who use ResourceArea also establish
ResourceMarks. E.g. Symbol::print_utf8_on().

But definitely, a comment in stringStream would help - warning the user
that the expandable version of stringStream uses ResourceArea.

Kind Regards, Thomas



> Reluctantly Reviewed.
>
> 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) {
>>>>
>>>>