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

[Bharadwaj Yadavalli]

Thanks for your reviews Vladimir and Christian.

I deleted the extra white space and updated the webrev (and saved the 
disk space :-))

Bharadwaj

On 11/27/2012 8:13 PM, Vladimir Kozlov wrote:
> I will push this changes.
>
> Thanks,
> Vladimir
>
> On 11/27/12 16:50, Christian Thalinger wrote:
>>
>> On Nov 27, 2012, at 4:04 PM, Bharadwaj 
>> Yadavalli<bharadwaj.yadavalli at oracle.com>  wrote:
>>
>>>
>>> Updated the webrev
>>>
>>> http://cr.openjdk.java.net/~bharadwaj/7092905/webrev_03/
>>>
>>> Added the ability in LogCompilation tool to print out live nodes 
>>> info from log file.
>>
>> src/share/vm/opto/parse1.cpp:
>>
>> +  if (log)  log->done("parse nodes='%d'  live='%d' memory='%d'",
>>
>> There are two spaces before live (I know it doesn't matter but it's a 
>> waste of disk space ;-).
>>
>> Otherwise this looks good.
>>
>> -- Chris
>>
>>>
>>> Please review.
>>>
>>> Thanks,
>>>
>>> Bharadwaj
>>>
>>> On 11/23/2012 9:19 AM, Vladimir Ivanov wrote:
>>>>>> if (_log != NULL) {
>>>>>> - _log->done("phase nodes='%d'", C->unique());
>>>>>> + _log->done("phase name='%s'", _phase_name);
>>>>>> }
>>>>>> Phase name is redundant here - it duplicates info from the head. 
>>>>>> But I
>>>>>
>>>>> It is not redundant with nested phases and big methods you don't 
>>>>> see the
>>>>> head so it is nice to see which phase ends.
>>>> Valid point. I'm not against this change, just looking at it from 
>>>> automated parsing perspective. Current format isn't human-friendly 
>>>> anyway :-)
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>>>> would leave nodes count as is and add live nodes count.
>>>>>
>>>>> Agree. It could be duplicated in the head of the following phase but
>>>>> more info is good.
>>>>>
>>>>> Vladimir K.
>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir Ivanov
>>>>>>
>>>>>> On 11/20/12 10:39 PM, Bharadwaj Yadavalli wrote:
>>>>>>>
>>>>>>> Please review the updated webrev at
>>>>>>> http://cr.openjdk.java.net/~bharadwaj/7092905/webrev_03/
>>>>>>>
>>>>>>> Hopefully I have addressed all the review comments/suggestions.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Bharadwaj
>>>>>>>
>>>>>>> On 11/9/2012 12:35 PM, Vladimir Kozlov wrote:
>>>>>>>> Bharadwaj,
>>>>>>>>
>>>>>>>> Nils just reminded me (for his changes) that you need to fix our
>>>>>>>> LogCompilation parsing tool to recognize new "unique='n'
>>>>>>>> live_tracked='n'" values (old was "nodes='n'"):
>>>>>>>>
>>>>>>>> src/share/tools/LogCompilation/
>>>>>>>>
>>>>>>>> Also live_tracked could be simple "live".
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> Vladimir Kozlov wrote:
>>>>>>>>> VerifyIdealNodeCount flag should be also notproduct since it 
>>>>>>>>> is used
>>>>>>>>> only in debug VM.
>>>>>>>>>
>>>>>>>>> The next assert is not needed because it duplicates assert in
>>>>>>>>> live_nodes():
>>>>>>>>>
>>>>>>>>> bool check_node_count(uint margin, const char* reason) {
>>>>>>>>> if (unique() + margin>  (uint)MaxNodeLimit) {
>>>>>>>>> + assert(dead_node_count()<= unique(), "number of dead nodes
>>>>>>>>> more than created");
>>>>>>>>>
>>>>>>>>> Also don't split the assert in live_nodes() into 2 lines and 
>>>>>>>>> maybe
>>>>>>>>> use err_msg_res() to print _unique and _dead_node_count 
>>>>>>>>> values. Also
>>>>>>>>> I think it should return unsigned int type value as unique() 
>>>>>>>>> method.
>>>>>>>>>
>>>>>>>>> Second Compile() constructor does not set _dead_node_count(0).
>>>>>>>>>
>>>>>>>>> count_live_nodes_by_graph_walk() and NotANode() methods are 
>>>>>>>>> now used
>>>>>>>>> only in debug code so put them under #ifdef ASSERT.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> print_missing_nodes() should call 
>>>>>>>>> count_live_nodes_by_graph_walk()
>>>>>>>>> instead of duplicating code. And it should print to _log itself.
>>>>>>>>> TracePhase() code will have only call to it:
>>>>>>>>>
>>>>>>>>> 3133 if (VerifyIdealNodeCount) {
>>>>>>>>> 3134 Compile::current()->print_missing_nodes();
>>>>>>>>> 3135 }
>>>>>>>>>
>>>>>>>>> And do not check VerifyIdealNodeCount in print_missing_nodes() to
>>>>>>>>> allow call it in debugger without VerifyIdealNodeCount set.
>>>>>>>>>
>>>>>>>>> I think VerifyIdealNodeCount code should be in ~TracePhase() 
>>>>>>>>> (end of
>>>>>>>>> phase).
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Vladimir
>>>>>>>>>
>>>>>>>>> Bharadwaj Yadavalli wrote:
>>>>>>>>>> Addressed comments on my previous RFR. Changes at
>>>>>>>>>> http://cr.openjdk.java.net/~bharadwaj/7092905/webrev_02/
>>>>>>>>>>
>>>>>>>>>> These changes are made to keep an (almost) accurate running 
>>>>>>>>>> count of
>>>>>>>>>> the reachable (live) flow graph nodes. This will result in a 
>>>>>>>>>> more
>>>>>>>>>> realistic node count for various phases of C2 to decide on 
>>>>>>>>>> whether
>>>>>>>>>> to proceed with optimizations or not. Prior to these changes, C2
>>>>>>>>>> bails out of compilation based on the number of nodes created 
>>>>>>>>>> which
>>>>>>>>>> typically larger than the number of reachable (live) nodes.
>>>>>>>>>>
>>