RFR: 8150103: Convert TraceClassPaths to Unified Logging

David Holmes david.holmes at oracle.com
Tue Feb 23 02:24:27 UTC 2016


Hi Max,

On 23/02/2016 9:00 AM, Max Ockner wrote:
> New webrev: http://cr.openjdk.java.net/~mockner/classpath.06/
> I have addressed the changes suggested by Coleen.

Looks good.

I see what you mean about the problem trying to combine:

   36   log_info(classpath)("type=%s ", type_name(type));
   37   ClassLoader::trace_class_path("add misc shared path ", path);

into one statement - I had overlooked the sprintf requirements. The best 
solution to that I think (but not for this conversion) would be to 
redefine trace_class_paths as:

trace_class_paths(const char* msg, ...)

and have callers include the "%s" and "name" explicitly, instead of this 
method printing a (constant string) message then a name.

Thanks,
David
-----

> 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
>>>
>


More information about the hotspot-runtime-dev mailing list