stringStream in UL and nested ResourceMarks

[Thomas Stüfe]

Hi Stefan,

I saw this, but I also see LogStreamNoResourceMark being used as a default
for the (trace|debug|info|warning|error)_stream() methods of Log. In this
form it is used quite a lot.

Looking further, I see that one cannot just exchange
LogStreamNoResourceMark with LogStreamCHeap, because there are hidden usage
conventions I was not aware of:

LogStreamNoResourceMark  is allocated with new() in create_log_stream().
LogStreamNoResourceMark is an outputStream, which is a ResourceObj. In its
current form ResourceObj cannot be deleted, so destructors for ResourceObj
child cannot be called.

So, we could not use malloc in the stringStream - or exchange stringStream
for bufferedStream - because we would need a non-empty destructor to free
the malloc'd memory, and that destructor cannot exist.

Looking further, I see that this imposes subtle usage restrictions for UL:

LogStreamNoResourceMark objects are used via "log.debug_stream()" or
similar. For example:

    codecache_print(log.debug_stream(), /* detailed= */ false);

debug_stream() will allocate a LogStreamNoResourceMark object which lives
in the resourcearea. This is a bit surprising, because "debug_stream()"
feels like it returns a singleton or a member variable of log.

If one wants to use LogStreamCHeap instead, it must not be created with
new() - which would be a subtle memory leak because the destructor would
never be called - but instead on the stack as automatic variable:

    LogStreamCHeap log_stream(log);
    log_stream.print("hallo");

I may understand this wrong, but if this is true, this is quite a difficult
API. I have two classes which look like siblings but LogStreamCHeap can
only be allocated on the local stack - otherwise I'll get a memory leak -
while LogStreamNoResourceMark gets created in the resource area, which
prevents its destructor from running and may fill the resource area up with
temporary stream objects if used in a certain way.

Have I understood this right so far? If yes, would it be possible to
simplify this?

Kind Regards, Thomas




On Wed, Jun 7, 2017 at 9:20 AM, Stefan Karlsson <stefan.karlsson at oracle.com>
wrote:

> Hi Thomas,
>
>
> On 2017-06-06 11:40, Thomas Stüfe wrote:
>
>> Hi all,
>>
>> In our VM we recently hit something similar to
>> https://bugs.openjdk.java.net/browse/JDK-8167995 or
>> https://bugs.openjdk.java.net/browse/JDK-8149557:
>>
>> A stringStream* was handed down to nested print functions which create
>> their own ResourceMarks and, while being down the stack under the scope of
>> that new ResourceMark, the stringStream needed to enlarge its internal
>> buffer. This is the situation the assert inside stringStream::write()
>> attempts to catch (assert(Thread::current()->current_resource_mark() ==
>> rm); in our case this was a release build, so we just crashed.
>>
>> The solution for both JDK-816795 and JDK-8149557 seemed to be to just
>> remove the offending ResourceMarks, or shuffle them around, but generally
>> this is not an optimal solution, or?
>>
>> We actually question whether using resource area memory is a good idea for
>> outputStream chuild objects at all:
>>
>> outputStream instances typically travel down the stack a lot by getting
>> handed sub-print-functions, so they run danger of crossing resource mark
>> boundaries like above. The sub functions are usually oblivious to the type
>> of outputStream* handed down, and as well they should be. And if the
>> allocate resource area memory themselves, it makes sense to guard them
>> with
>> ResourceMark in case they are called in a loop.
>>
>> The assert inside stringStream::write() is not a real help either, because
>> whether or not it hits depends on pure luck - whether the realloc code
>> path
>> is hit just in the right moment while printing. Which depends on the
>> buffer
>> size and the print history, which is variable, especially with logging.
>>
>> The only advantage to using bufferedStream (C-Heap) is a small performance
>> improvement when allocating. The question is whether this is really worth
>> the risk of using resource area memory in this fashion. Especially in the
>> context of UL where we are about to do expensive IO operations (writing to
>> log file) or may lock (os::flockfile).
>>
>> Also, the difference between bufferedStream and stringStream might be
>> reduced by improving bufferedStream (e.g. by using a member char array for
>> small allocations and delay using malloc() for larger arrays.)
>>
>> What you think? Should we get rid of stringStream and only use an
>> (possibly
>> improved) bufferedStream? I also imagine this could make UL coding a bit
>> simpler.
>>
>
> Not answering your questions, but I want to point out that we already have
> a UL stream that uses C-Heap:
>
> logging/logStream.hpp:
>
> // The backing buffer is allocated in CHeap memory.
> typedef LogStreamBase<bufferedStream> LogStreamCHeap;
>
> StefanK
>
>
>
>> Thank you,
>>
>> Kind Regards, Thomas
>>
>>