RFR(XL): 8181917: Refactor UL LogStreams to avoid using resource area
thomas.stuefe at gmail.com
Wed Jun 21 07:29:57 UTC 2017
thank you for reviewing!
Delta to last version:
- classfile/loaderConstraints.cpp: as you suggested, I fixed up all cases
of "superfluous LogStream usage" I found and converted them to direct
- 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
Thanks & Regards, Thomas
On Mon, Jun 19, 2017 at 2:30 PM, Marcus Larsson <marcus.larsson at oracle.com>
> Hi Thomas,
> 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.
> 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
> 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); 59
> Should these be uncommented?
> Thanks again,
More information about the hotspot-dev