RFR: 8150103: Convert TraceClassPaths to Unified Logging

[Coleen Phillimore]

This looks good!
Coleen

On 2/22/16 6:00 PM, Max Ockner wrote:
> New webrev: http://cr.openjdk.java.net/~mockner/classpath.06/
> I have addressed the changes suggested by Coleen.
>
> Comments below:
>
> On 2/22/2016 12:43 AM, David Holmes wrote:
>> 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?
>>
> This definitely seems like an issue that needs to be worked out. As 
> far as I know we have not heard anything about when it is safe 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.
>
> In sharedPathsMiscInfo.cpp, we have this:
>
> void SharedPathsMiscInfo::add_path(const char* path, int type) {
>   log_info(classpath)("type=%s ", type_name(type));
>   ClassLoader::trace_class_path("add misc shared path ", path);
>   write(path, strlen(path) + 1);
>   write_jint(jint(type));
> }
>
> We would need to evaluate the format string in the log_info call 
> before passing into trace_class_path(). Something like this (from 
> ./closed/share/vm/classfile/classLoaderExt.cpp)
>
>       int name_len = (int)strlen(file_start);
>       if (name_len > 0) {
>         ResourceMark rm(THREAD);
>         char* libname = NEW_RESOURCE_ARRAY(char, dir_len + name_len + 1);
>         *libname = 0;
>         strncat(libname, dir_name, dir_len);
>         strncat(libname, file_start, name_len);
>         trace_class_path("library = ", libname);
>
> David, is this what you were thinking? I personally think think this 
> looks clunky, but if it's needed to preserve and important property of 
> the output, then it shall be done.
>>
>> 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
>>>
>