Request for reviews (S): 6991188: C2 Crashes while compiling method

[Y. S. Ramakrishna]

Sounds good; thanks for your patient explanation, Vladimir! :-)

-- ramki

On 11/03/10 11:33, Vladimir Kozlov wrote:
> Y. S. Ramakrishna wrote:
>>
>>
>> On 11/03/10 10:16, Vladimir Kozlov wrote:
>>> Thank you, Ramki
>>>
>>> How about this?:
>>>
>>>   // Normally only 1-3 passes needed to build
>>>   // Connection Graph depending on graph complexity.
>>>   // Set limit to 10 to catch situation when something
>>>   // did go wrong and recompile the method without EA.
>>> #define CG_BUILD_ITER_LIMIT 10
>>>   int iterations = 0;
>>>   while(_progress && (iterations++ < CG_BUILD_ITER_LIMIT)) {
>>
>> I suppose it is not possible to say "The maximum number of
>> times we might need to iterate in the worst case is bounded
>> above by k*n, where k is the foo-metric of the graph and
>> n is its bar-metric", or ".. where foo is the number of
>> deferred edges and bar is the number of
>> nodes of type AddBaz in the starting graph", or some such
>> structural complexity metric/property of the code or the graph being
>> transformed?
> 
> No. You can't calculate how many iterations you will need.
> 
>>
>> What you have above looks good otherwise. (You have another use of
>> 10 after the wile loop, at line 1610, which i am guessing you would 
>> also change to use the defined constant.)
> 
> Correct, I also added #undef:
> 
>   if (iterations >= CG_BUILD_ITER_LIMIT) {
>     // Possible infinite build_connection_graph loop.
>     // Retry compilation without escape analysis.
>     C->record_failure(C2Compiler::retry_no_escape_analysis());
>     _collecting = false;
>     return false;
>   }
> #undef CG_BUILD_ITER_LIMIT
> 
>>
>> Also, what does the #iterations bound? The cost of the Escape Analysis,
>> or the size of the resulting code or both? (i.e. what kind of cost is
>> the iteration limit bounding?) It might be nice to mention that (unless
>> that is considered obvious to those familiar with the code).
> 
> As new comment say iterations bound check tries to catch situations
> when after 10 iterations new graph edges are still added when we
> expect that graph construction should be finished after, say, no more
> than 4 (I never saw 4) iterations in 99.99% cases.
> Reaching 10 iterations could mean 2 things:
> 1. Something did go wrong, for example bug in the EA code, which
> cause infinite edges adding. We need to break out from such situation.
> 2. The java code is very complex and it really needs more than 10
> iterations to construct Connection Graph. We want to bail out
> it also since it would, most likely, cause reaching Ideal nodes number
> limit and we bail out later anyway.
> 
> Thanks,
> Vladimir
> 
>>
>> Ah, may be there is no trivial upper bound, for you do say "possible 
>> infinite
>> build_connection_graph loop" at line 1611, which i missed on my
>> previous reading. That does make some of my comments above about
>> upper bound and cost-limiting somewhat moot.
>>
>> Sorry again for my naive and nit-picky-sounding comments, stemming 
>> from my
>> lack of any bkgrd or knowledge of this code, so feel free to take all
>> of this with the right dose of salt (i.e. with lots of it) :-)
>>
>> Thanks.
>> -- ramki
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> Y. S. Ramakrishna wrote:
>>>> Hi Vladimir --
>>>>
>>>> Just a general $0.02 comment, without my knowing anything about
>>>> what is actually being done here.
>>>>
>>>> Can you give an upper bound on the number of iterations
>>>> that would be needed in terms of a suitable metric on the
>>>> starting graph? A little more commentary on the upper bound
>>>> (if a non-trivial one exists), and the choice and suitability of
>>>> the default, would be nice.
>>>>
>>>> Also, perhaps use a defined constant instead of the 10? (Would it
>>>> be useful to have it be a diagnostic tunable?)
>>>>
>>>> -- ramki
>>>>
>>>> On 11/02/10 16:59, Tom Rodriguez wrote:
>>>>> On Nov 2, 2010, at 4:42 PM, Vladimir Kozlov wrote:
>>>>>
>>>>>> Thanks, Tom
>>>>>>
>>>>>> Tom Rodriguez wrote:
>>>>>>> Typo:
>>>>>>> +     // Possible infinit build_connection_graph loop.
>>>>>> Fixed.
>>>>>>
>>>>>>> Why 10?  How many passes does it normally take? 
>>>>>> Normally only 1-2 passes depending on graph complexity. In the bug 
>>>>>> test - 2, in JBB I saw 3 once.
>>>>>>
>>>>>>> Could this process only the nodes which changed or does it really 
>>>>>>> need to reprocess every node?
>>>>>> Only AddP, LoadP and StoreP (and narrow variants) are reprocessed. 
>>>>>> All other nodes are marked in vectSet _processed. There is check 
>>>>>> at the beginning of build_connection_graph().
>>>>>> I thought about using a separate worklist in new iterating code 
>>>>>> and populate it only with A/L/S nodes but I don't think it will 
>>>>>> help much.
>>>>>> And we need to reprocess all A/L/S nodes since we don't know which 
>>>>>> nodes will be affected by one change (the chain through deferred 
>>>>>> edges could be long and split/merge through Phis).
>>>>>
>>>>> Sounds and looks good.
>>>>>
>>>>> tom
>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>>> tom
>>>>>>> On Nov 2, 2010, at 3:27 PM, Vladimir Kozlov wrote:
>>>>>>>> http://cr.openjdk.java.net/~kvn/6991188/webrev.00
>>>>>>>>
>>>>>>>> Fixed 6991188: C2 Crashes while compiling method
>>>>>>>>
>>>>>>>> Construction of Connection Graph during EA relied on
>>>>>>>> DU relation corresponds to nodes index so that def
>>>>>>>> nodes processed before their uses. Move EA after
>>>>>>>> IGVN (for 6966411 fix broke that. Because of that
>>>>>>>> not all Graph edges will be created leading to
>>>>>>>> incorrect EA results and incorrect Ideal graph.
>>>>>>>>
>>>>>>>> Walk over interesting nodes (AddP, LoadP, StoreP)
>>>>>>>> several times until no new edges are created.
>>>>>>>> Set limit on iterations.
>>>>>>>>
>>>>>>>> Tested with assert in iterations bailout code and
>>>>>>>> calls to PhaseIdealLoop::verify() (removed from
>>>>>>>> final changes). Passed JPRT, full CTW, nsk.
>>>>>>>>
>>>>>