RFR(XL): 8181917: Refactor UL LogStreams to avoid using resource area
Thomas Stüfe
thomas.stuefe at gmail.com
Tue Jul 4 10:05:07 UTC 2017
Hi Erik,
On Mon, Jul 3, 2017 at 4:19 PM, Erik Helin <erik.helin at oracle.com> wrote:
> 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*...
>
>
Fair enough. I retried and got a ton of unhelpful linkage errors on
Windows, basically almost every file has a reference to LogStream::operator
delete(). Retried on Linux, and gcc refuses to compile, always with a
variant of the following:
<quote>
In file included from
/priv/d031900/openjdk/jdk10-hs/source/hotspot/test/native/logging/logTestUtils.inline.hpp:26:0,
from
/priv/d031900/openjdk/jdk10-hs/source/hotspot/test/native/logging/test_logTagSetDescriptions.cpp:25:
/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/share/vm/logging/logStream.hpp:
In destructor ‘virtual LogStreamTemplate<(LogLevel::type)5u,
(LogTag::type)50u, (LogTag::type)0u, (LogTag::type)0u, (LogTag::type)0
u, (LogTag::type)0u, (LogTag::type)0u>::~LogStreamTemplate()’:
/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/share/vm/logging/logStream.hpp:61:6:
error: ‘static void LogStream::operator delete(void*)’ is private
void operator delete(void* p);
^
In file included from
/priv/d031900/openjdk/jdk10-hs/source/hotspot/test/native/logging/logTestUtils.inline.hpp:26:0,
from
/priv/d031900/openjdk/jdk10-hs/source/hotspot/test/native/logging/test_logTagSetDescriptions.cpp:25:
/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/share/vm/logging/logStream.hpp:99:7:
error: within this context
class LogStreamTemplate : public LogStream {
^
</quote>
First I wanted to know if this is a remnant of LogStream being a
ResourceObj, so I removed the inheritance between outputStream and
ResourceObj, but the same errors happen (beside new ones, of course).
I believe this is caused by ~outputStream() being virtual - so a virtual
~LogStreamTemplate() gets synthesized, which wants to reference
LogStream::operator delete()? I have the vague notion that this has
something to do with the fact that operator delete is "static but
polymorphic" in the sense that for polymorphic delete calls it needs to be
looked up via the virtual destructor. Something like this: "
https://stackoverflow.com/questions/2273187/why-is-the-delete-operator-required-to-be-static".
However, there are lots of things I do not understand here.
I still do not think this is a problem, since we disallow new() sucessfully.
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).
>
>
I think it can be done, but I am not sure it is worth the trouble.
Right now, LogOutput is implemented by only one concrete child, writing to
a FILE*. I would not swap a single IO syscall (fprintf) against a number of
them to save some memory reallocations. So, we still call fprintf() only
once, and we would have to assemble all the chunked buffers beforehand.
Granted, that still could be a bit better than reallocating the LogStream
line buffer every 64byte, but that also depends on the line buffer
allocation strategy (back to doubling the memory for the line buffer?)
Thinking further ahead: Assuming that we want to implement more LogOutput
childs in the future, I would be careful adding an API which exposes
lockinf behaviour of the underlying implementation. This means I would not
expose the sequence of functions ("lock(), write(),...,write(), unlock()")
in the general LogOutput() API (just as an example, lets assume you want to
have a LogOutput writing to a lock-free ring buffer in shared memory or
some such).
But I do not think you meant that, right? You meant something like a single
call, "log(struct my_list_of_chunks_t*)" ? I think that could be done, but
if we find that all possible LogOutput implementations we can think up will
re-assemble the chunks anyway before doing something with it, it may not be
worth the trouble.
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 :)
>
>
Yes. I think the backend could be a bit optimized to minimize copying.
> 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
>
>
>
Thanks! ..Thomas
> ...Thomas
>>
>>
>>
>> Thanks,
>> Erik
>>
>> Kind Regards, Thomas
>>
>>
>>
More information about the hotspot-dev
mailing list