RFR(XL): 8181917: Refactor UL LogStreams to avoid using resource area
marcus.larsson at oracle.com
Mon Jun 19 13:37:11 UTC 2017
On 2017-06-19 15:16, Thomas Stüfe wrote:
> Hi Marcus,
> On Mon, Jun 19, 2017 at 2:30 PM, Marcus Larsson
> <marcus.larsson at oracle.com <mailto: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:
> (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
> 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().
> I found some of those too, but can a sequence of LogStream::print()
> calls really be replaced by a sequence of
> LogStream buffers all input till newline is encountered, whereas
> LogTarget::print (LogImpl::vwrite()->LogTagSet::vwrite()...) will
> print one log line for each invocation, no?
Indeed they can. There's nothing special going on in LogStreams to make
the lines grouped in any way. Each print_cr on a LogStream is equivalent
to a LogImpl::write call.
> 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.
> Yes, you are right, and it is also not enclosed in is_enabled(). Also,
> seems this coding is never tested, otherwise the assert in
> ~LogStreamBase() would have fired because of missing stream flush().
True. The old code would never had a chance to hit the assert due to the
resource allocation destructor problem, but now we should be able to hit
it with proper testing.
> 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
> Ok! I'll add whatever I find as comments.
>> 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?
> Oh, right. I have to play a bit more, was unsure whether that coding
> actually does what I want it to do, prevent ResourceObj::operator new
> from running. Will test a bit more.
Alright, sounds good.
> Thanks again,
> Thanks for your review! I'll prepare an updated patch.
More information about the hotspot-dev