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

[harold seigel]

Hi Max,

Looks good, just a few comments:

1. The copyright for BadMap50.jasm needs to be corrected.

2. Can the TRAPS parameter be removed from log_end_verification() ?

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