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

[Vladimir Kozlov]

On 11/21/12 04:52, Vladimir Ivanov wrote:
> Few minor comments:
>
> src/share/vm/opto/compile.cpp:
> +static inline bool NotANode(const Node* n) {
> Can you rename it to something like not_a_node? Camel style names aren't
> common in the codebase.
>
> 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.

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