RFR(XL): 8181917: Refactor UL LogStreams to avoid using resource area

[Erik Helin]

On 06/30/2017 08:32 PM, Thomas Stüfe wrote:
> New
> Version: http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor-ul-logstream/all.webrev.03/webrev/
>
> Delta to
> last: http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor-ul-logstream/delta-02-to-03/webrev/

Thanks, I will take a look later.

On 06/30/2017 08:32 PM, Thomas Stüfe wrote:
>     - should you add declarations of delete as well to prevent someone from
>       calling delete on LogStream pointer?
>
>
> Tried this, but I think I ran into linker errors. I do not think it is
> needed, though.

Hmm, that seems strange, a linker error would/could indicate that some 
code _is_ calling delete on a LogStream*...

On 06/30/2017 08:32 PM, Thomas Stüfe wrote:
>     - is 128 bytes as default too much for _smallbuf? Should we start with
>       64?
>
> I changed it to 64.

Ok.


On 06/30/2017 08:32 PM, Thomas Stüfe wrote:
>     logStream.cpp
>     - one extra newline at line 74
>     - the pointer returned from os::malloc is not checked. What should we
>       do if a NULL pointer is returned? Print whatever is buffered and then
>       do vm_out_of_memory?
>     - is growing by doubling too aggressive? should the LineBuffer instead
>       grow by chunking (e.g. always add 64 more bytes)?
>
>
> Okay. I changed the allocation of the line buffer such that
> - we do not allocate beyond a reasonable limit (1M hardwired), to
> prevent runaway leaks.
> - we cope with OOM
> In both cases, LogStream continues to work but may truncate the output-
>
> I also added a test case for the former scenario (the 1M cap).

Ok, sounds good, I will have a look.

On 06/30/2017 08:32 PM, Thomas Stüfe wrote:
>     - instead of growing the LineBuffer by reallocating and then copying
>       over the old bytes, should we use a chunked linked list instead (in
>       order to avoid copying the same data multiple times)? The only
>       "requirements" on the LineBuffer is fast append and fast iteration
>       (doesn't have to support fast index lookup).
>
>
> Not really sure how this would work. The whole point of LogStream is to
> assemble one log line, in the form of a single continuous
> zero-terminated string, and pass that to the UL backend as one single
> log call, yes? How would you do this with disjunct chunks?

The key point here is "as one single log call". If we allow ourselves to 
pass a more elaborate data structure than a plain string (such as a 
linked list of strings), then we can enable this kind of "chunked 
expansion". The write call will eventually result in

   flockfile(_stream);
   // do stuff
   fflush(_stream);
   funlockfile(_stream);

deep down in logFileStreamOutput.cpp. Whatever we do at
`// do stuff` will become atomic wrt to other threads. We could for 
example iterate over a linked list of strings and print them (with space 
as separator). For an example, see 
LogFileStreamOutput::write(LogMessageBuffer::Iterator msg_iterator).

The worst case for this patch is if the log line becomes larger than 512 
bytes. In this case, there will be plenty of copying as LineBuffer gets 
expanded and then LogTagSet::vwrite will also allocate a buffer on the 
heap, copy everything over (again), print that buffer, free it. It seems 
like we can do better here :)

Whether this should be done as part of this patch, or as a follow-up 
patch, that is another matter.

>     - LogStream::write is no longer in inline.hpp, is this a potential
>       performance problem? I think not, ,the old LogStream::write
>       definition was most likely in .inline.hpp because of template usage
>
>
> I do not think so either. I do not like implementations in headers
> (clutters the interface), unless it is worth it performance wise. Here,
> LogStream::write is usually called from one of the
> outputStream::print()... functions via virtual function call; not sure
> how that could even be inlined.
>
>
>
>     Great work thus far Thomas, the patch is becoming really solid! If
>     you want to discuss over IM, then you can (as always) find me in
>     #openjdk on irc.oftc.net <http://irc.oftc.net>.
>
>
> Thanks :) And thanks for reviewing!

Thanks,
Erik

> ...Thomas
>
>
>
>     Thanks,
>     Erik
>
>         Kind Regards, Thomas
>
>