RFR(XL): 8181917: Refactor UL LogStreams to avoid using resource area
Erik Helin
erik.helin at oracle.com
Tue Jun 27 12:53:45 UTC 2017
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
- should you add declarations of delete as well to prevent someone from
calling delete on LogStream pointer? 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.
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.
- is 128 bytes as default too much for _smallbuf? Should we start with
64?
- the keyword `public` is repeated unnecessarily LogStream class (a
superfluous `public` before the write method)
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)?
- 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).
- 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
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,
Erik
> Kind Regards, Thomas
More information about the hotspot-dev
mailing list