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

[Erik Helin]

On 07/04/2017 12:05 PM, Thomas Stüfe wrote:
>     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.

Ok.

On 07/04/2017 12:05 PM, Thomas Stüfe wrote:
>     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?)

Yeah, I agree that doing the syscall multiple times is probably more 
expensive than the copying the memory.

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

Yes, this is what I was thinking of. Such a
log(struct my_list_of_chunks_t*) could then do the final allocation and 
copy everything over once, then call fprintf.

This is probably not worth doing for this patch, but I think we should 
file an enhancement for this and look into it later.

I will continue reviewing the rest of the patch!

Thanks,
Erik