Request for reviews (XL): 7147744: CTW: assert(false) failed: infinite EA connection graph build

[Vladimir Kozlov]

Thank you very much, Christian

I updated changes:

http://cr.openjdk.java.net/~kvn/7147744/webrev.1/

Christian Thalinger wrote:
> On Feb 23, 2012, at 8:58 PM, Vladimir Kozlov wrote:
> 
>> http://cr.openjdk.java.net/~kvn/7147744/webrev
> 
> src/share/vm/opto/escape.cpp:
> 
>  255       for (uint j = 0; j < ucnt; j++) {
>  259           for (uint j = 0; j < acnt; j++) {
> 
> You should probably use different loop variable names.

Done.

> 
> 2217   for (uint i = 0; i < count; i++) {
> 2233     for (DUIterator_Fast imax, i = n->fast_outs(imax); i < imax; i++) {
> 2258       for (uint i = 0; i < bcnt; i++) {
> 
> Same here.

Done.

> 
>  385   // If we have already computed a value, return it.
>  386   if (es >= PointsToNode::GlobalEscape)
>  387     return false;
>  388 
>  389   if (ptn->is_JavaObject()) {
>  390     return (es < PointsToNode::GlobalEscape);
>  391   }
> 
> The compare on 390 is always true given the if on 386.

You are right. I switched order of checks but did not realize that it could be 
simplified after that.

> 
> 1746   while ((new_edges > 0) &&
> 1747         (iterations++   < CG_BUILD_ITER_LIMIT) &&
> 1748         (time.seconds() < CG_BUILD_TIME_LIMIT)) {
> 1749     time.start();
> 1750     new_edges = 0;
> 1751     // Propagate references to phantom_object for nodes pushed on _worklist
> 1752     // by find_non_escaped_objects().
> 1753     new_edges += add_java_object_edges(phantom_obj, false);
> 1754     for (uint next = 0; next < java_objects_length; ++next) {
> 1755       JavaObjectNode* ptn = java_objects_worklist.at(next);
> 1756       new_edges += add_java_object_edges(ptn, true);
> 1757     }
> 1758     if (new_edges > 0) {
> 1759       // Update escape states on each iteration if graph was updated.
> 1760       if (!find_non_escaped_objects(ptnodes_worklist, non_escaped_worklist)) {
> 1761         _collecting = false;
> 1762         return false; // Nothing to do.
> 1763       }
> 1764     }
> 1765     time.stop();
> 1766   }
> 
> Can you try to refactor this code because you're using it again almost identical a few lines later?

I put it inside "do { } while (new_edges > 0)" which encloses the above loop and 
find_field_value() code. So that if find_field_value() added new edges to 
phantom object the above loop will be executed again.

> 
> 3094         ; // nothing to do
> 
> This looks weird.

I added more explanation:

     default:
       // During first iteration (when all ideal nodes are scanned) do nothing
       // for nodes not related to EA. During second iteration this method
       // should be called only for EA specific nodes.
       if (!first_iteration) {
         ShouldNotReachHere();
       }

> 
> src/share/vm/opto/escape.hpp:
> 
>  423         return false; // alredy points to phantom_obj
>  443         return false; // alredy has phantom_obj base
> 
> Typos.

Fixed.

> 
> src/share/vm/opto/phase.cpp:
> 
>  108       tty->print_cr ("      escape analysis: %3.3f sec", Phase::_t_escapeAnalysis.seconds());
>  109       tty->print_cr ("        build connection graph: %3.3f sec", Phase::_t_buildConnectionGraph.seconds());
>  110       tty->print_cr ("      macroEliminate : %3.3f sec", Phase::_t_macroEliminate.seconds());
> 
> It would be easier to read if the time output would be aligned.

It is intentional to indicate that "build connection graph" phase is part of EA:

   Phases:
     parse          : 1.419 sec
     optimizer      : 4.523 sec
       escape analysis: 2.144 sec
         build connection graph: 2.139 sec
       macroEliminate : 0.001 sec
       iterGVN        : 0.213 sec

How about this?:

   Phases:
     parse          : 1.419 sec
     optimizer      : 4.523 sec
       escape analysis: 2.144 sec
         connection graph: 2.139 sec
       macroEliminate : 0.001 sec
       iterGVN        : 0.213 sec

> 
> src/share/vm/utilities/growableArray.hpp:
> 
> +   void delete_at(int index) {
> 
> Given the discussion on the list maybe add a comment that the ordering changes.

Done.

> 
> I think the EA code is good but the changes are too big to be sure.

I did not change split unique type code for scalar replaceable allocations. I 
did want but hold back :)

Thanks,
Vladimir

> 
> -- Chris
> 
>> 7147744: CTW: assert(false) failed: infinite EA connection graph build
>>
>> I rewrote Connection graph construction code in EA to reduce time spent there. In the bug's test time reduced by 100 (from about 50 to .5 sec on Nahalem-EX machine).
>>
>> Connection graph now has specialized classes for nodes and additional use edges to put on worklist only uses of node which added new point edge. Field node has also bases edges. Edges never removed only added.
>> Instead of looking for Field's bases from the start create simple base edge to LocalVar during initial graph construction in build_connection_graph(). Late do several iteration to push all known JavaObject nodes references through graph. This phase has limits on number and time. Also on each iteration check if there are non globally escaped objects and bail out from code if not.
>> Added additional Arraycopy node to connect source and destination objects.
>> I removed uncast() calls so that all LocalVar nodes point to all related JavaObject nodes.
>> I combined record_for_escape_analysis() and build_connection_graph() into one method.
>> Added TracePhase around Connection graph build code to see how much time spent there.
>>
>> This code need addition work since I still saw outlier (10 min in EA) in sje2010 on SPARC. But I will look on it after this one is done.
>>
>> I added new GrowableArray method delete_at(i) to avoid shifting following elements which is done in remove_at(i) method.
>>
>> Thanks,
>> Vladimir
>