RFR: 8150720: Cleanup code around PrintOptoStatistics

[Vladimir Kozlov]

Clearing nearby code is good. You missed {} in ifnode.cpp (no need to 
resend webrev)

+  if (this == dom)
      return NULL;

Yes, changes in node.cpp are good.

Thanks,
Vladimir

On 2/26/16 3:19 PM, Claes Redestad wrote:
> Hi,
>
> On 2016-02-26 18:56, Vladimir Kozlov wrote:
>> Good changes. We indeed print statistics only in non-product build.
>
> Thanks!
>
>>
>> graphKit.cpp - add {} to code:
>>
>> +#ifndef PRODUCT
>>      if (null_true == top())
>>        explicit_null_checks_elided++;
>> +#endif
>>
>> ifnode.cpp - add {} and adjust spaces "if (dist > 2) {":
>>
>> +#ifndef PRODUCT
>>    if( dist > 2 )              // Add to count of NULL checks elided
>>      explicit_null_checks_elided++;
>> +#endif
>>
>> parse1.cpp - the same:
>>
>> -  if( implicit_null_throws )
>> +  if( SharedRuntime::_implicit_null_throws )
>>      tty->print_cr("%d implicit null exceptions at runtime",
>> -                  implicit_null_throws);
>> +                  SharedRuntime::_implicit_null_throws);
>
> Sure! Mind if I clean up nearby to keep things consistent?
>
> http://cr.openjdk.java.net/~redestad/8150720/webrev.01/
>
>>
>>
>> > There is also a small "leak" in the corner case where "edge_end ==
>> > (char*)this" since we're not freeing the input array in this case.
>>
>> There is no "leak" since after compilation is done all C2 arenas are
>> cleaned/freed. Yes, during compilation required space could be increased.
>>
>> > We are not freeing when ASSERT is not defined? O_o The patch that
>> > apparently does an equivalent transformation of the original code
>> makes it even creepier.
>>
>> It is done to catch dangling Node* pointers in debug build:
>>
>>  661 #ifdef ASSERT
>>  662   // We will not actually delete the storage, but we'll make the
>> node unusable.
>>  663   *(address*)this = badAddress;  // smash the C++ vtbl, probably
>>  664   _in = _out = (Node**) badAddress;
>>
>> So please revert that code back except the code which just increment
>> unused counters and can be indeed removed.
>
> Thanks for clarifying and explaining this!
>
> I do think the code in node.cpp is correctly transformed to do the same
> thing apart from the counters and does not need to be reverted though?
>
> Thanks!
>
> /Claes
>
>>
>> Thanks,
>> Vladimir
>>
>> On 2/26/16 3:40 AM, Claes Redestad wrote:
>>> Hi,
>>>
>>> src/share/vm/opto/parse1.cpp currently defines a number of global
>>> counters that is used to collect some statistics. Some of these counters
>>> are incremented in product builds, but ever only displayed in
>>> non-product builds if run with -XX:+PrintOptoStatistics.
>>>
>>> This patch masks such counters from product builds, removes a few
>>> counters that were never read, fixes an issue with implicit_null_throws
>>> which was never incremented in current code (seems it was moved to
>>> SharedRuntime a long time ago without updating print_statistics) and
>>> removes an unused enum (InlineStyle):
>>>
>>> webrev: http://cr.openjdk.java.net/~redestad/8150720/webrev.00/
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8150720
>>>
>>> Testing: RBT --test hotspot/test/:hotspot_all, manual verification that
>>> -XX:+PrintOptoStatistics prints out a count for implicit NPE's thrown
>>>
>>> Thanks!
>>>
>>> /Claes
>