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

[Tobias Hartmann]

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