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

[Max Ockner]

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