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

[Max Ockner]

Here is a new webrev: http://cr.openjdk.java.net/~mockner/ulclassinit05/

1) The verifier test uses a small snippet to produce verifier errors and 
a discussion with Harold made it seem like this was the most direct way 
to hit most of the error messages.

2) In general I do not know where the best home is for the logging 
tests. I think we can have a separate discussion about this if people 
find it to be a problem.

3) You caught a bug in my test! I have addressed this issue. 
http://cr.openjdk.java.net/~mockner/ulclassinit05/test/logging/ClassInitializationTest.java.html

Thanks,
Max



On 11/25/2015 7:51 PM, David Holmes wrote:
> Hi Max,
>
> On 25/11/2015 7:27 AM, Max Ockner wrote:
>> New webrev @ http://cr.openjdk.java.net/~mockner/ulclassinit02/
>> Fixed everything that I said I would fix below.
>
> As a direct conversion of existing logging info this looks fine. The 
> overlap with VerboseVerification will have to be dealt with later.
>
> Not sure about the test strategy here in that creating a class that 
> fails verification just to check what is logged seem a bit over the 
> top and potentially duplicates verifier testing. I wonder if the 
> logging test could not in fact run an existing verifier test with 
> logging enabled? Which raises the question of whether logging tests 
> like this come under logging, or under the subsystem being logged?
>
> In the test EagerInitialization is a develop-only flag, and 
> VerboseVerification is diagnostic. It isn't obvious this is being 
> handled by the createJavaProcessBuilder calls.
>
> Thanks,
> David
>
>>
>> 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
>>>
>>