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

[David Holmes]

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) {
>>>>>