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

David Holmes david.holmes at oracle.com
Fri Jan 29 02:46:06 UTC 2016


On 29/01/2016 12:39 PM, Coleen Phillimore wrote:
> 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());

Perhaps. And/Or we could have simplified macro forms that don't 
double-up on the is-enabled check. The current doubling-up of the check 
seems to be prevalent in the GC logging code too.

David

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


More information about the hotspot-runtime-dev mailing list