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

Max Ockner max.ockner at oracle.com
Fri Jan 29 16:59:40 UTC 2016


Thanks for reviewing.
Comments below:

On 1/28/2016 7:50 PM, 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.

David gave a good answer for this question. We need to check 
log_is_enabled() before defining the resource mark. Once inside this 
scope, we could choose to define a log stream and then print to it, or 
we could use the macro log_trace, which also happens to check 
log_is_enabled again. I vote for keeping log_trace because it is cleaner 
than the alternative.

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

I spoke with Ioi about this. The logs in filemap.cpp are really part of 
TraceClassPaths and not part of TraceClassLoading.

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

I found two other lines that needed to be broken up. I split up any line 
I found that was longer than 80 chars.

>
> - 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(?).

Correct. "-Xlog:classload=info" should print the same thing as 
"-XX:+TraceClassLoading".  If you also set the Verbose flag with 
TraceClassLoading or TraceClassUnloading, you can see additional output. 
This output can be accessed with "-Xlog:classload=debug". However, the 
Verbose flag is only available in non-product builds. The logging alias 
table is meant to preserve the product functionality of 
TraceClassLoading and TraceClassUnloading, so there is nothing that gets 
aliased to the debug level for classload or classunload.
>
> 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
>>
>>



More information about the hotspot-runtime-dev mailing list