RFR(XL): 8181917: Refactor UL LogStreams to avoid using resource area
marcus.larsson at oracle.com
Mon Jun 19 12:30:11 UTC 2017
Thanks for your effort on this, great work!
On 2017-06-18 09:30, Thomas Stüfe wrote:
(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
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.
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
> 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. :)
56 // Prevent operator new for LogStream.
57 // static void* operator new (size_t);
58 // static void* operator new (size_t);
Should these be uncommented?
More information about the hotspot-dev