RFR: 8145934: Make ttyLocker equivalent for Unified Logging framework

[Marcus Larsson]

Hi Thomas,

On 02/11/2016 09:27 AM, Thomas Stüfe wrote:
> Hi Marcus,
>
> thank you for implementing this!

Thanks for looking at it!

>
> I am happy that you implement this with a temporary buffer rather than 
> implementing a lock.
>
> I did not yet look closely at all changes, this is quite a large 
> change. But I have some questions about the memory you use:
>
> You use either resource area or NMT-tracked C heap. In both cases this 
> is quite "high level memory" in the sense that it uses a number of 
> components. These components then cannot use logging (or at least the 
> LogMessage) without introducing circular references. Worse, this may 
> often work and only crash if the LogMessage::grow() function grows the 
> buffer, so it is difficult to test.
>
> Would it not be better to use plain low-level raw malloc in this case?
>
> Or, alternativly, factor memory allocation out into an own interface 
> to be used as template parameter, so that most people get a reasonable 
> default:
>
> LogMessage myMessage;
> ..
>
> but low level components like NMT, for example, could do:
>
> LogMessage<MyRawMallocator> myMessageUsingRawMalloc;
>
> ?

Good point. This is actually already an issue with the current 
implementation for regular log messages. If they are longer than what 
the stack buffer in Log<>::vwrite() can hold, a buffer will be allocated 
on the heap instead (NMT tracked). I'm considering to approach this as a 
follow-up issue though, given that it's a pre-existing issue and that it 
won't be a problem until logging for NMT or similar is added.

>
> A second concern with using ResourceArea: One has to be careful with 
> handing down LogMessage objects to subroutines, to fill the LogMessage 
> object, and further down the callstack happens another ResourceMark 
> destroying the LogMessage buffer. Could that be a problem, and if yes, 
> could we have an assert for this case?

Yes, there is a problem here if the LogMessage buffer grows under a 
different ResourceMark. I think it's possible to add an assert here, but 
I wonder if we don't actually want something more robust than that. 
Perhaps skipping the resource allocations altogether is a better idea.

Thanks,
Marcus

>
> Kind Regards, Thomas
>
>
>
>
>
>
> On Wed, Feb 10, 2016 at 2:49 PM, Marcus Larsson 
> <marcus.larsson at oracle.com <mailto:marcus.larsson at oracle.com>> wrote:
>
>     Hi,
>
>     Please review the following patch adding support for
>     non-interleavable multi-line log messages in UL.
>
>     Summary:
>     This patch adds a LogMessage class that represents a multiline log
>     message, buffering lines that belong to the same message. The
>     class has a similar interface to the Log class, with printf-like
>     methods for each log level. These methods will append the log
>     message with additional lines. Once all filled in, the log message
>     should be sent to the the appropriate log(s) using Log<>::write().
>     All lines in the LogMessage are written in a way that prevents
>     interleaving by other messages. Lines are printed in the same
>     order they were added to the message (regardless of level). Apart
>     from the level, decorators will be identical for lines in the same
>     LogMessage, and all lines will be decorated.
>
>     Webrev:
>     http://cr.openjdk.java.net/~mlarsson/8145934/webrev.00/
>     <http://cr.openjdk.java.net/%7Emlarsson/8145934/webrev.00/>
>
>     Issue:
>     https://bugs.openjdk.java.net/browse/JDK-8145934
>
>     Testing:
>     Included tests through JPRT
>
>     Thanks,
>     Marcus
>
>