RFR: 8150103: Convert TraceClassPaths to Unified Logging

[David Holmes]

Hi Max,

On 20/02/2016 6:11 AM, Max Ockner wrote:
> I have responded to these suggestions.
> New webrev: http://cr.openjdk.java.net/~mockner/classpath.05/
>
>   - ClassLoader::trace_class_path() no longer takes a stream argument.
> Because the body of trace_class_path is guarded by the classpath tag
> (formerly TraceClassPaths), the output should always go to
> LogHandle(classpath)::info_stream().
>   - SharedPathMiscInfo::trace_class_path() has been removed since it is
> now redundant. Its only purpose was to call trace_class_path on tty.
> ClassLoader::trace_class_path is now called in its place.
>   - print_path() no longer takes a stream argument. It now prints to
> LogHandle(classpath)::info_stream().
>   - Accessing LogHandle(classpath)::info_stream() was causing issues
> with the nonexistant current thread in arguments.cpp, so the single
> logging statement in arguments.cpp was moved to classLoader.cpp.

That seems like an issue with UL - do we have any clear guidance at what 
point in VM initialization we can start to use UL?

src/share/vm/classfile/sharedPathsMiscInfo.cpp

I agree with Coleen that the changes in check() should be simplified, 
and also in add_path(). I think the strings that were printed outside of 
trace_class_path and print_paths, should just be folded in to what gets 
passed to trace_class_paths/print_paths, as they appear to be prefixes - 
they used tty->print not tty->print_cr.

Thanks,
David
-----

> Thanks,
> Max
>
> On 2/18/2016 7:28 AM, David Holmes wrote:
>> Hi Max,
>>
>> This conversion is not straight-forward, and not complete.
>>
>> On 18/02/2016 8:19 AM, Max Ockner wrote:
>>> Hello everyone,
>>> Please review this conversion of TraceClassPaths to UL.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8150103
>>> Webrev: http://cr.openjdk.java.net/~mockner/classpath.03/
>>
>> src/share/vm/classfile/classLoader.cpp
>>
>> This call:
>>
>>  477     trace_class_path(tty, "[Bootstrap loader class path=",
>> sys_class_path);
>>
>> shouldn't be passing tty but should be the outputstream for the "info"
>> log.
>>
>> ---
>>
>> src/share/vm/runtime/arguments.cpp
>>
>> Same here:
>>
>>   if (!PrintSharedArchiveAndExit) {
>>     ClassLoader::trace_class_path(tty, "[classpath: ",
>> _java_class_path->value());
>>   }
>>
>> ---
>>
>> src/share/vm/classfile/sharedPathsMiscInfo.hpp
>>
>> You didn't modify this file but you need to change the 2-arg
>> trace_class_path() to not use the tty.
>>
>> This conversion seems a bit problematic to me. trace_class_path was
>> enabled/disabled via TraceClassPaths, but it could write to different
>> output streams. With UL you would always want it to write to the
>> single UL related stream - which means the API really needs to change,
>> and the extraction of the correct log stream can then be internalized.
>> But there may be some compatibility issues in doing that for the
>> place(s) where it may not be writing to tty.
>>
>> I don't see a simple conversion process here. I think all
>> trace_class_path calls should be the two-arg forms with no stream arg,
>> and that version then inlines the current 3-arg version but always
>> writes to the info log stream. But then you also have print_path which
>> wraps a call to trace_class_path ... to be honest I don't see what
>> purpose print_path serves, it just seems to be complicating things.
>>
>> David
>> ----
>>
>>
>>> Summary:  TraceClassPaths flag has been reimplemented using the
>>> classpath tag with Info level.  TraceClassPaths has been added to the
>>> logging alias table.
>>> There is some related output that was never guarded by "if
>>> (TraceClassPaths)", so it has not been included in this conversion.
>>> Testing: jtreg runtime tests. Added basic test case at
>>> test/runtime/logging/ClassPathTest.java
>>>
>>> Thanks,
>>> Max
>