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

[Max Ockner]

Whoops, thought I replied earlier.

Webrev has been updated again. Same link: 
http://cr.openjdk.java.net/~mockner/ulclassinit03

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