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

[Marcus Larsson]

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/ 
> <http://cr.openjdk.java.net/%7Estuefe/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/ 
> <http://cr.openjdk.java.net/%7Estuefe/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/ 
> <http://cr.openjdk.java.net/%7Estuefe/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