[9] RFR(M): 8040213: C2 does not put all modified nodes on IGVN worklist

[Tobias Hartmann]

Hi Vladimir,

On 21.07.2014 19:13, Vladimir Kozlov wrote:
> > Thanks for the hint, the optimized build did not work. I'm now using
> > PRODUCT_RETURN and NOT_PRODUCT to guard the verification code. This is
> > consistent with other verification methods like
> > Compile::verify_graph_edges(..).
>
> The logic which verifies modified nodes in phaseX.cpp is under #ifdef 
> ASSERT. Yes, you are not collecting data in optimized VM by returning 
> NULL from modified_nodes(). But it is very confusing. I would suggest 
> to move all this code to debug build only. You can use 
> NOT_DEBUG_RETURN instead of PRODUCT_RETURN and put definitions under 
> #ifdef ASSERT in compile.cpp file.

Okay, that's true. I missed that there is a NOT_DEBUG_RETURN. Now using it.

New webrev: http://cr.openjdk.java.net/~thartmann/8040213/webrev.04/

Thanks,
Tobias

>
> Thanks,
> Vladimir
>
> On 7/21/14 2:50 AM, Tobias Hartmann wrote:
>> Vladimir, John, thanks for the review.
>>
>> On 18.07.2014 01:32, John Rose wrote:
>>> On Jul 17, 2014, at 3:32 PM, Vladimir Kozlov
>>> <vladimir.kozlov at oracle.com> wrote:
>>>
>>>> Initial prototype was written by me so part of changes are mine.
>>>>
>>>> On 7/17/14 3:40 AM, Tobias Hartmann wrote:
>>>>> Hi John,
>>>>>
>>>>> thanks a lot for the detailed review! Please see comments inline.
>>>>>
>>>>> On 16.07.2014 01:10, John Rose wrote:
>>>>>>>> Done. New webrev:
>>>>>>>> http://cr.openjdk.java.net/~thartmann/8040213/webrev.01/
>>>>>> Loading C->_modified_nodes with a stack address is bad news, even if
>>>>>> the code apparently cleans it up on exit.  (I say "apparently" 
>>>>>> because
>>>>>> there are early exits from that block, and the cleanup is not RAII
>>>>>> style.)  I strongly suggest heap allocation instead of stack
>>>>>> allocation; it's expensive but it's for debug code.
>>>> The list array allocation is done in compiler arena which is cleaned
>>>> after compilation finished by ResourceMark, only descriptor is on 
>>>> stack:
>>>>
>>>> +   Unique_Node_List modified_nodes(comp_arena());
>>>>
>>>> It is our normal style. We do it a lot, see Compile::Code_Gen().
>>> Yes, we do it when adding phase control blocks to the Compile object,
>>> within the tree of calls rooted at the constructor.  I see that the
>>> new code is arguably part of that pattern.  I hope that is an
>>> exceptional use of this style?
>>>
>>> Note that C2Compiler::compile_method creates the Compile object and
>>> then queries it.  When it queries it, the Compile object may contain
>>> dangling pointers into phase control blocks stored on the stack
>>> (during subroutine calls like Code_Gen) but now deallocated.
>>>
>>> It seems to me that we avoid touching those dangling pointers by
>>> simple will power and discipline.   Such a style doesn't scale, since
>>> it's too easy to make mistakes as the code gets complicated.
>>>
>>>> Tobias, if you want to allocate Unique_Node_List object on arena:
>>>>
>>>> _modified_nodes = new (comp_arena()) Unique_Node_List(comp_arena());
>>> That makes me feel much better.
>>>
>>> I would prefer for us to do it this way for the other stack-chunks
>>> also, but that would be outside the scope of this bug.
>>
>> I changed it to heap allocation.
>>
>>> Tobias, thanks for doing this work.
>>>
>>> — John
>>>
>>>>> There are other places in the code where this is done, for example in
>>>>> Compile::Compile(..) the Unique_Node_List 'for_igvn' is created on 
>>>>> the
>>>>> stack. Is this a special case?
>>>>>
>>>>> I'm not sure how to correctly allocate the object on the heap. Do I
>>>>> have
>>>>> to use new/delete or is this handled by some magic in 
>>>>> "ResourceObject'?
>>>>>
>>>>>
>>>>>> You inserted '#ifdef ASSERT' after
>>>>>> 'PhaseIterGVN::verify_PhaseIterGVN', but I don't think you need it,
>>>>>> since all of the code (including the similar insertion a few 
>>>>>> lines up)
>>>>>> is '#ifndef PRODUCT'.  See also suggestion above regarding
>>>>>> 'modified_nodes'.
>>>> #ifndef PRODUCT is also 'optimized' version of VM. n->dump() method
>>>> is not defined in optimized build. That is why asserts are needed.
>>>>
>>>> Tobias, please, build 'optimized' VM to verify your changes.
>>
>> Thanks for the hint, the optimized build did not work. I'm now using
>> PRODUCT_RETURN and NOT_PRODUCT to guard the verification code. This is
>> consistent with other verification methods like
>> Compile::verify_graph_edges(..).
>>
>> New webrev: http://cr.openjdk.java.net/~thartmann/8040213/webrev.03/
>>
>> Thanks,
>> Tobias
>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>> True, I removed the #ifdef.
>>>>>
>>>>>> There are a few places where tests like 'while
>>>>>> (modified_list->size())' appears but there is no null check of
>>>>>> 'modified_list' itself.  This could cause a crash if we ever get the
>>>>>> phasing wrong; consider an assert or explicit null check 'while
>>>>>> (modified_list != NULL && ...)'.
>>>>> I added explicit null checks.
>>>>>
>>>>>
>>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8040213/webrev.02/
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>
>>>>>> — John