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

[Vladimir Ivanov]

>> 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.
>>>>>>