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

Mon Feb 1 15:50:59 UTC 2016

On 2/1/2016 6:35 AM, David Holmes wrote:
> On 30/01/2016 2:32 AM, Max Ockner wrote:
>> David,
>> Thanks for explaining why it is necessary to check log_is_enabled().
>> However I think it clouds up the code to define a logstream and print to
>> it when we could use log_trace, and provides minimal benefit in the
>> process.
>
> I personally hate redundant checks but ...
>
> I'm more concerned that we are being quite inconsistent in how we are 
> using the logging API. A macro that can internalize the ResourceMark 
> would be beneficial I think - then we can go back and clean up many of 
> the existing log_is_enabled checks.
Just to add my two cents as another logging person, I agree that a macro 
with a ResourceMark would improve consistency and conciseness, in 
addition to making it easier to understand and use the UL framework. Not 
to take this email thread too far off topic.

R
>
> Thanks,
> David
>
>> Thanks,
>> Max
>>
>> On 1/28/2016 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));
>>>
>>>
>>> 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
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>