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

[Vladimir Kozlov]

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