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

[Vladimir Kozlov]

This is very nice.

Thanks,
Vladimir

On 11/27/12 16:04, Bharadwaj Yadavalli 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.
>
> 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.
>>>>>>>>