RFR: 8142976: TraceClassInitialization has been reimplemented with Unified Logging.

[harold seigel]

Looks good.

Thanks, Harold

On 11/30/2015 4:25 PM, Max Ockner wrote:
> Hopefully the last correction: 
> http://cr.openjdk.java.net/~mockner/ulclassinit04
>
> Max
>
> On 11/30/2015 2:51 PM, Coleen Phillimore wrote:
>>
>>
>> On 11/30/15 2:16 PM, harold seigel wrote:
>>> Hi Max,
>>>
>>> Looks good, just a few comments:
>>>
>>> 1. The copyright for BadMap50.jasm needs to be corrected.
>>
>> BadMap50.jasm needs the same copyright as ClassInitializationTest.java
>>>
>>> 2. Can the TRAPS parameter be removed from log_end_verification() ?
>>
>> The TRAPS parameter is to pass THREAD so this function can check 
>> HAS_PENDING_EXCEPTION (which it doesn't clear) so it seemed 
>> appropriate to keep it with the last TRAPS parameter.
>>
>> Unless you think passing Thread* THREAD would be better.
>>
>> Thanks,
>> Coleen
>>
>>>
>>> Thanks, Harold
>>>
>>> On 11/30/2015 1:22 PM, Max Ockner wrote:
>>>> New webrev: http://cr.openjdk.java.net/~mockner/ulclassinit03/
>>>> Fixed everything mentioned by Coleen.
>>>>
>>>> Max
>>>>
>>>> On 11/24/2015 7:17 PM, Coleen Phillimore wrote:
>>>>>
>>>>> Hi Max,  This looks mostly good:
>>>>>
>>>>> http://cr.openjdk.java.net/~mockner/ulclassinit02/src/share/vm/classfile/verifier.cpp.udiff.html 
>>>>>
>>>>>
>>>>> In function log_end_verification, the indentation is wrong. It 
>>>>> should be 2 and the end } should be in column 1.
>>>>>
>>>>> You didn't mention that we decided that if you specify 
>>>>> VerboseVerification *and* -Xlog:classinit that you'll get the same 
>>>>> message twice. This is because the logs can go to different 
>>>>> places. When VerboseVerification is converted to UL, the logging 
>>>>> statements will be more compact.
>>>>>
>>>>> Can you fix the indentation of this line too?
>>>>>
>>>>> *!tty->print_cr("Fail over class verification to old verifier for: 
>>>>> %s", klassName);*
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~mockner/ulclassinit02/test/logging/BadMap50.jasm.html 
>>>>>
>>>>>
>>>>> This has the wrong copyright header.
>>>>>
>>>>> http://cr.openjdk.java.net/~mockner/ulclassinit02/test/logging/ClassInitializationTest.java.html 
>>>>>
>>>>>
>>>>>   30  * @run driver ClassInitializationTest
>>>>>
>>>>>
>>>>> Is "driver" new?
>>>>>
>>>>> You don't need to include these:
>>>>>
>>>>>   34 import java.lang.ref.WeakReference;
>>>>>   35 import java.lang.reflect.Method;
>>>>>
>>>>>
>>>>> Otherwise, I think this looks good.
>>>>>
>>>>> Coleen
>>>>>
>>>>> On 11/24/15 4:27 PM, Max Ockner wrote:
>>>>>> New webrev @ http://cr.openjdk.java.net/~mockner/ulclassinit02/
>>>>>> Fixed everything that I said I would fix below.
>>>>>>
>>>>>> On 11/24/2015 3:40 PM, Rachel Protacio wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Looks mostly good, just a few comments:
>>>>>>>
>>>>>>> verifier.cpp
>>>>>>> - at line 118 (the first "Verification for" line), it should 
>>>>>>> just be "print", not "print_cr".
>>>>>> Thanks. Fixed, though I wonder how much it matters.
>>>>>>> - between lines 194 and 195, I think you need a ResourceMark for 
>>>>>>> the LogHandle stream.
>>>>>> The ResourceMark is defined already, it just isn't part of the 
>>>>>> diffs because it was already there.
>>>>>>> - in the sections starting at lines 179 and 608, I appreciate 
>>>>>>> that you were minimizing the number of lines, but I think it's a 
>>>>>>> bad idea to have duplicates of the logged strings. Do you think 
>>>>>>> you could define the strings outside of the logging and pass it 
>>>>>>> to both functions? Another possible solution would be to make a 
>>>>>>> function to do that with a signature like
>>>>>>>     void log_multiple(bool enabled1, outputStream* st1, bool 
>>>>>>> enabled2, outputStream* st2, char* msg);
>>>>>>> that could do this in a more formalized manner. A function like 
>>>>>>> this could be useful for other similar situations as well while 
>>>>>>> we're converting flags one by one. Or what are your thoughts on 
>>>>>>> that?
>>>>>> Two reasons why I don't think we should do that.
>>>>>>     (1) We don't want to evaluate format strings unless something 
>>>>>> is being logged. I guess if you can find a way to avoid doing 
>>>>>> this while still making the code look nicer then that is OK. I 
>>>>>> think it would be just as bad to write extra lines of code just 
>>>>>> to ensure that a short string isn't duplicated.
>>>>>>     (2) We do not guarantee that these two messages will always 
>>>>>> be the same. A conversation I had with Coleen led me to believe 
>>>>>> we should keep the messages separate.
>>>>>>> - I think the reordered nesting makes sense.
>>>>>>>
>>>>>>> ClassInitializationTest.java
>>>>>>> - nit: can you move the ");"s from the process builder lines 
>>>>>>> onto the lines before them?
>>>>>> OK.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Rachel
>>>>>>>
>>>>>>> On 11/24/2015 3:09 PM, Max Ockner wrote:
>>>>>>>> Hello,
>>>>>>>> Please review my new unified logging code:
>>>>>>>>
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8142976
>>>>>>>> http://cr.openjdk.java.net/~mockner/ulclassinit01/src/share/vm/classfile/verifier.cpp.cdiff.html 
>>>>>>>>
>>>>>>>> Summary: -XX:+TraceClassInitialization logging has been 
>>>>>>>> reimplemented using unified logging under the classinit tag.
>>>>>>>>
>>>>>>>> In the segment with recursive verification (see verifier.cpp) I 
>>>>>>>> reordered the nested if statement to check 
>>>>>>>> was_recursively_verified() first. I valued clean code over 
>>>>>>>> potentially avoiding a function call to was_recursively_verified.
>>>>>>>>
>>>>>>>> What do you think?
>>>>>>>>
>>>>>>>> Tested with:
>>>>>>>> jtreg hotspot tests
>>>>>>>> new jtreg test for classinit tag
>>>>>>>> performance testing with refworkload.
>>>>>>>>
>>>>>>>> Thanks, Max
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>