Request for Reviews(M): 7092905: C2: Keep track of the number of dead nodes

[Vladimir Kozlov]

And use consistent codding style for "if ()" and "for ()" - space before () and 
no space inside (): "for (uint".

Vladimir

Vladimir Kozlov wrote:
> I forgot to tell that VerifyIdealNodeCount flag should be defined in 
> c2_globals.hpp since it is C2 specific flag.
> 
> Also how you handle Node::destruct() mathod which may allow to reuse _idx?
> 
> The rest comments are inlined below.
> 
> Bharadwaj Yadavalli wrote:
>>
>> Thanks for your reviews, Vladimir and Christian.
>>
>> On Oct 29, 2012, at 10:29 AM, Vladimir Kozlov 
>> <vladimir.kozlov at oracle.com> wrote:
>>>> Bharadwaj,
>>>>
>>>> These changes look much better. Sorry, I suggested to add 
>>>> verify_live_node_count() method before but your current code don't 
>>>> need it, just use count_live_nodes_by_graph_walk().
>>
>> OK. Made the necessary changes.
>>
>>>> An other note: without LogCompilation verification code should 
>>>> produce some info on tty.
>>
>> OK. We do not need to print the information about the number of unique 
>> nodes created, those that are live and those that are dead to the tty, 
>> correct? (Ref. around line 777 and line 816 in compile.cpp)
> 
> It is useful information when you try to diagnose node limit bailouts. 
> You can add new debug flag PrintIdealNodeCount to output this info to tty.
> 
>>
>> That leaves us with the message when there is a mismatch between the 
>> live node count arrived at by flow-graph walk and that kept track of 
>> via deal_node_list.
>>
>> Would you suggest that I print out to tty indicating a mismatch? (Ref. 
>> around line 819 in compile.cpp) A mismatch may not always indicate 
>> that something is indeed wrong - given that some constant nodes that 
>> become unreachable (dead) can later become reachable, for example, due 
>> to final_graph_reshaping_walk(). Hence I do not count constant nodes 
>> that become unreachable as dead. So printing out a message on the tty 
>> might trigger false alarms. So, I chose to print a message in the log 
>> file only.
> 
> -XX:+VerifyIdealNodeCount is our internal flag and we would expect these 
> messages to be not accurate.
> 
>>
>> Please let me know if you still think that I should print this info 
>> (the one around line 819 in compile.cpp) to tty.
> 
> Yes, also you need to dump nodes when output is tty instead of printing 
> their idx:
> 
>  useful.at(i)->dump();
> 
> Thanks,
> Vladimir
> 
>>
>> On 10/29/2012 1:56 PM, Christian Thalinger wrote:
>>> We still need to adjust MaxNodeLimit.  Maybe some JRuby or Nashorn 
>>> benchmarks can help in getting really big methods.
>>
>> I agree. However, I felt that arriving at the MaxNodeLimit might need 
>> some experimentation with different applications whose method 
>> compilations result in creation of graphs with large number of nodes. 
>> I do not (yet) know how the current value was arrived at. It appears 
>> to me that leaving the current value unchanged in light of these 
>> proposed changes would not adversely affect normal applications. So, I 
>> thought we could check in these changes and later check in a change to 
>> MaxNodeLimit based on a detailed experimentation. What do you think?
>>
>>> src/share/vm/opto/compile.cpp:
>>>
>>> +     _log->elem("node count before optimize compile_id='%d' 
>>> unique='%d' live (tracked)='%d' live(graph_walk)='%d'",
>>>
>>> I think the log element name needs to have _'s so we can match it (if 
>>> we want to):
>>>
>>> +     _log->elem("node_count_before_optimize compile_id='%d' 
>>> unique='%d' live (tracked)='%d' live(graph_walk)='%d'",
>>
>> OK. Changed.
>>
>> Thanks,
>>
>> Bharadwaj
>>