RFR: 8150720: Cleanup code around PrintOptoStatistics

[Claes Redestad]

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