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

[Thomas Stüfe]

Hi Eric,

thank you for the review!

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/

Please find comments inline.


On Tue, Jun 27, 2017 at 2:53 PM, Erik Helin <erik.helin at oracle.com> wrote:

> On 06/21/2017 06:16 PM, Thomas Stüfe wrote:
>
>> New Webrev:
>>  http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor
>> -ul-logstream/all.webrev.02/webrev/
>>
>
> I have spent most of my time in log.hpp, logStream.{hpp,cpp}, so I will
> start with those comments/questions and then continue reviewing the
> callsites.
>
> log.hpp:
> - two extra newlines near end of file
>
> logStream.hpp:
> - one extra newline near end of file
>

I cleaned up all extra newlines I added.


> - 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.


> Thinking about this, it seems a
>   bit odd to pass a function an outputStream* (a pointer to a
>   ResourceObj) that doesn't obey the ResourceObj contract. An
>   outputStream* coming from &ls, where ls is an LogStream instance on
>   the stack, is a pointer to a ResourceObj that in practice is not a
>   ResourceObj.
>

Yes I agree. Deriving outputStream from ResourceObj seems an odd choice but
as far as I can tell it has always been this way and I did not want to
change it. Especially since, as you noted, the contract is not really tight
:) You still can place them on the stack or allocate them from C-Heap.


>   Thinking about this some more, I think this is safe. No function that
>   is passed an outputStream* can assume that it can call delete on that
>   pointer.
>
>
Ok!


> - is 128 bytes as default too much for _smallbuf? Should we start with
>   64?
>

I changed it to 64.


> - the keyword `public` is repeated unnecessarily LogStream class (a
>   superfluous `public` before the write method)
>
>
Fixed.


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



> - 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?

- 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.
>
>
Thanks :) And thanks for reviewing!

...Thomas


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