RFR: 8079408: Reimplement TraceClassLoading, TraceClassUnloading, and TraceClassLoaderData with Unified Logging.

[Coleen Phillimore]

On 1/28/16 8:01 PM, David Holmes wrote:
> On 29/01/2016 10:50 AM, Jiangli Zhou wrote:
>> Hi Max,
>>
>> The changes look very good. I have just few minor comments and 
>> questions below.
>>
>> - klass.cpp
>> I’m new to the logging code, so I might be wrong. It seems the ‘if 
>> (log_is_enabled(Trace, classunload))’ checks at line 390 & 405 are 
>> not needed since log_trace() also checks if the trace flag is enabled.
>
> Good catch - we need the "if (log_is_enabled())" check to ensure the 
> ResourceMark is only defined when logging is enabled, but then the 
> actual log statement should be the direct form, not the log_xxx() 
> macro form. E.g as used here for exceptions logging:
>
>       if (log_is_enabled(Info, exceptions)) {
>         ResourceMark rm;
>         outputStream* logstream = LogHandle(exceptions)::info_stream();
>         logstream->print("Async. exception installed at runtime exit 
> (" INTPTR_FORMAT ")", p2i(this));
>

I think I had him change it to be the more compact looking form of 
log_info(classload)("message") .  I don't think the implementation 
details need to be exposed just to avoid testing the logging tag twice.  
It would be nice to have a log macro that also had a resource_mark 
enclosed.  We have a lot of these.

log_info_rm(classload)("message %s", foo->external_name());

Coleen

>
> David
>
>> - filemap.cpp
>> ‘TraceClassLoading && Verbose’ is removed at line 211. Should a 
>> ‘log_info(classload)’ call be added? Or maybe the skipping of 
>> classloading trace for shared path is intentional?
>>
>> The same question for other changes in filemap.cpp.
>>
>> - instanceKlass.cpp
>> Please break line 2908 into two lines so it’s not too long. There are 
>> a few other long lines in print_loading_log() should be broken into two.
>>
>> - arguments.cpp
>> LogLevel:Info is added for TraceClassLoading and TraceClassUnloading 
>> in aliased_logging_flags[]. Should there be one for LogLevel::Debug 
>> for the classloading tracing? Maybe there is no old styled option for 
>> debug level class loading trace, so it's not defined in 
>> aliased_logging_flags(?).
>>
>> Thanks,
>> Jiangli
>>
>>
>>> On Jan 28, 2016, at 11:29 AM, Max Ockner <max.ockner at oracle.com> wrote:
>>>
>>> Forwarded private discussion with Ioi about changing the test to use 
>>> ClassUnloadCommon.
>>>
>>> Hopefully nearing the end:
>>>
>>> Webrev: http://cr.openjdk.java.net/~mockner/classload.08/
>>>
>>> -------- Original Message --------
>>> Subject:     Re: RFR: 8079408: Reimplement TraceClassLoading, 
>>> TraceClassUnloading, and TraceClassLoaderData with Unified Logging.
>>> Date:     Thu, 28 Jan 2016 10:57:15 -0800
>>> From:     Ioi Lam <ioi.lam at oracle.com>
>>> To:     Max Ockner <max.ockner at oracle.com>
>>>
>>>
>>>
>>> Hi Max,
>>>
>>> Thanks for doing this. Also, I sent the e-mail to you personally, so 
>>> you may want to post the new version in the open list for record 
>>> purpose.
>>>
>>> Thanks
>>> - Ioi
>>>
>>> On 1/28/16 8:11 AM, Max Ockner wrote:
>>>> Ioi,
>>>> I have replaced my test with this test that you have given me 
>>>> (thank you!)
>>>> I also have removed the commented out section which used to test 
>>>> for "making nmethod ...", and I have added the comment that you 
>>>> suggested at the top of ClassUnloadCommon.java
>>>> Thanks,
>>>> Max
>>>> On 1/27/2016 7:13 PM, Ioi Lam wrote:
>>>>>
>>>>>
>>>>> On 1/27/16 12:51 PM, Max Ockner wrote:
>>>>>> Though Ioi suggested I change my new test, I have not done that. 
>>>>>> I was recommended to copy from 
>>>>>> runtime/ClassUnload/UnloadTest.java instead of rolling my own 
>>>>>> test for class unloading. I mentioned that it was tricky to make 
>>>>>> the new test work, but it was tricky because I was trying to copy 
>>>>>> from UnloadTest.java. This test refers to a class "test.Empty" 
>>>>>> from a "classes" library, but the new test has a processBuilder 
>>>>>> which I think does not play nicely with the class path for  
>>>>>> "test.Empty". In the end it was much easier to hardcode the 
>>>>>> entire test into one place than to follow UnloadTest.java and 
>>>>>> refer to extra libraries.
>>>>>>
>>>>>
>>>>> Hi Max,
>>>>>
>>>>> I've written a version of ClassLoadUnloadTest.java using 
>>>>> ClassUnloadCommon. The main trick is here:
>>>>>
>>>>>         Collections.addAll(argsList, "-Dtest.classes=" + 
>>>>> System.getProperty("test.classes","."));
>>>>>
>>>>> That's because of this line in ClassUnloadCommon:
>>>>>
>>>>>             return new URLClassLoader(new URL[] {
>>>>> Paths.get(System.getProperty("test.classes",".") + 
>>>>> File.separatorChar + "classes").toUri().toURL(),
>>>>>             }, null);
>>>>>
>>>>> When Jtreg launches a sub-process, it strips off all the Java 
>>>>> system properties, so you need to add the "-D" in the command-line.
>>>>>
>>>>> I also disabled the test related to "making nmethod ... 
>>>>> unloadable". As Rachel found out in JDK-8146137, the compiler is 
>>>>> unpredictable. Even if you invoke a method 10001 times, there's no 
>>>>> guaranteed that it will always be compiled. Since this is not an 
>>>>> 'easy-to-test' feature, we should just skip it.
>>>>>
>>>>> I would also suggest adding this to the header of 
>>>>> ClassUnloadCommon.java
>>>>>
>>>>> /*
>>>>> * To use ClassUnloadCommon from a sub-process, see 
>>>>> hotspot/test/runtime/logging/ClassLoadUnloadTest.java
>>>>> * for an example.
>>>>> */
>>>>>
>>>>> What do you think?
>>>>> - Ioi
>>>>
>>>
>>>
>>>
>>