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

[Thomas Stüfe]

Hi Marcus,

thank you for reviewing!

New webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor-ul-logstream/all.webrev.01/webrev/

Delta to last version:
http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor-ul-logstream/delta-all-00-to-01/webrev/

Changes:

- classfile/loaderConstraints.cpp: as you suggested, I fixed up all cases
of "superfluous LogStream usage" I found and converted them to direct
LogTarget::print() calls.

- classfile/sharedPathsMiscInfo.cpp: here I opted for removing any notion
of UL from this method, instead I just hand in an outputStream*. Both the
"is_enabled" check and the LogStream creation is now handed in the caller
frame. I also added a trailing cr().

- gc/cms/compactibleFreeListSpace.cpp: removed the superfluous ResourceMark

- logging/logStream.hpp: enabled the private operator new() declarations to
disable heap allocations for class LogStream. I also gave it a try, works
fine, if you do new(), now you get a linker error.

The rest of the changes is concerned with the removal of "LogStreamCHeap"
which is not needed anymore. Note that I found some new instances of
"unguarded printing" and I updated comments at JDK-8182466
<https://bugs.openjdk.java.net/browse/JDK-8182466> .

Thanks & Regards, Thomas


On Mon, Jun 19, 2017 at 2:30 PM, Marcus Larsson <marcus.larsson at oracle.com>
wrote:

> Hi Thomas,
>
> Thanks for your effort on this, great work!
>
> On 2017-06-18 09:30, Thomas Stüfe wrote:
>
>
> --
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor-ul-logstream/
> callsite-changes.webrev.00/webrev/
>
>
> (Although this is a pre-existing issue, it might be a good opportunity to
> clean it up now.)
> In file loaderConstraints.cpp, class LoaderConstraintTable, for functions
> purge_loader_constraints(), add_entry(), check_or_update(),
> extend_loader_constraint() and merge_loader_constraints():
> A LogStream is created, but only ever used for print_cr():s, which sort of
> defeats its purpose. It would be much simpler just to use the LogTarget
> directly. This is actually what's done for the converted
> log_ldr_constraint_msg().
>
> A similar but worse issue is present in sharedPathsMiscInfo.cpp:
> Here, a LogStream is used to print incomplete lines without any CR at the
> end. These messages will never be logged. Also, the use of a stream here is
> unnecessary as well.
>
> In compactibleFreeListSpace.cpp:
>
> 2200       ResourceMark rm;
>
> It should be safe to remove this ResourceMark.
>
> These are the - mostly mechanical - changes to the many callsites. Most of
> these changes follow the same pattern. A code sequence using "xxx_stream()"
> was split into declaration of LogTarget or Log (usually the former), an
> "is_enabled()" query and declaration of LogStream on the stack afterwards.
> This follows the principle that the logging itself should be cheap if
> unused: the declaration of LogTarget is a noop, because LogTarget has no
> members. Only if is_enabled() is true, more expensive things are done, e.g.
> initializing the outputStream object and allocating ResourceMarks.
>
> Note that I encountered some places where logging was done without
> enclosing "is_enabled()" query - for instance, see gc/g1/heapRegion.cpp, or
> cms/compactibleFreeListSpace.cpp. As far as I understand, in these cases
> we actually do print (assemble the line to be printed), only to discard all
> that work in the UL backend because logging is not enabled for that log
> target. So, we pay quite a bit for nothing. I marked these questionable
> code sections with an "// Unconditional write?" comment and we may want to
> fix those later in a follow up issue?
>
>
> That sounds good to me. I found more sites where the logging is
> unconditional (compactibleFreeListSpace.cpp, parOopClosures.inline.hpp,
> g1RemSet.cpp), but we should fix them all as a separate issue. I filed
> https://bugs.openjdk.java.net/browse/JDK-8182466.
>
>
> --
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8181917-
> refactor-ul-logstream/api-changes.webrev.00/webrev/
>
> The API changes mostly are simplifications. Before, we had a whole
> hierarchy of LogStream... classes whose only difference was how the backing
> memory was allocated. Because we now always use C-Heap, all this can be
> folded together into a single simple LogStream class which uses Cheap as
> line memory. Please note that I left "LogStreamNoResourceMark" and
> "LogStreamCHeap" for now and typedef'ed them to the one single LogStream
> class; I will fix those later as part of this refactoring.
>
>
> Looks good to me, as long as we get rid of the typedefs too eventually. :)
>
>
> --
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8181917-refactor-ul-logstream/
> logstream-optimization.webrev.00/webrev/
>
>
>   56   // Prevent operator new for LogStream.  57 //  static void* operator new (size_t);  58 //   static void* operator new[] (size_t);  59
>
> Should these be uncommented?
>
> Thanks again,
> Marcus
>
>