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

[Vladimir Kozlov]

Thank you, Tom

Vladimir

Tom Rodriguez wrote:
> Sounds good.
> 
> tom
> 
> On Mar 10, 2012, at 10:12 AM, Vladimir Kozlov wrote:
> 
>> Windows build failed because it has already macro FAILED :)
>>
>> C:\jprt\temp\P1\003453.vkozlov\source\src\share\vm\opto\escape.cpp(515) : warning C4005: 'FAILED' : macro redefinition
>>        C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\include\winerror.h(23781) : see previous definition of 'FAILED'
>>
>> I will rename the macro back to ELSE_FAIL but leave the code the same (no else in #define):
>>  if () {
>>    break;
>>  }
>>  ELSE_FAIL("foo");
>>
>> Thanks,
>> Vladimir
>>
>> On 3/9/12 11:01 AM, Vladimir Kozlov wrote:
>>> Here are additional changes based on your review after webrev.3:
>>>
>>> http://cr.openjdk.java.net/~kvn/7147744/webrev.diff3
>>>
>>> I decided to move PointsToNode allocation to comp_arena. Changes are not big (I need to pass Compile* to constructors to
>>> allocate edges GrowableArray in comp_arena). So I added ResourceMark into do_analysis() method.
>>>
>>>>>> Node* const _node;
>>>> That's the second one. The first is a pointer to a const Node and you want a const pointer to a Node. It binds to the
>>> left unless there's a type name to the right.
>>>
>>> You are right, I changed it to "Node* const".
>>>
>>>>> const Type *aat = igvn->type(arg);
>>>> But isn't it available as _igvn?
>>> Yes, you are absolutely right, I missed it. It seems, I need to wear glasses :) It is from old time when I did not cache
>>> _igvn. There are other methods to which such argument is passed also. I fixed it.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> Tom Rodriguez wrote:
>>>> On Mar 8, 2012, at 7:29 PM, Vladimir Kozlov wrote:
>>>>
>>>>> Thank you, Tom
>>>>>
>>>>> Tom Rodriguez wrote:
>>>>>> The structure of the code is much improved I think. Thanks for taking the time to fix it.
>>>>>> Why aren't all the worklists part of ConnectionGraph? There doesn't seem to be a clear split between state that's
>>>>>> needs just the EA computation which is transient and the stuff that's recorded in the connection graph. It would be
>>>>>> nice if all the stuff allocated just for the analysis could be freed once it's done.
>>>>> All worklists created in compute_escape() could be freed, that is why they are not part of ConnectionGraph object.
>>>>> But I can't use ResourceMark here since EA graph nodes (PointsToNode) are ResourceObj and they are needed now after
>>>>> EA is done. I have separate rfe 7017319 to free resources which do not needed after analysis is done. So for these
>>>>> changes I decided to not to bother with resources freeing.
>>>> Ok.
>>>>
>>>>>> Why do we ever care around JavaFields that aren't oops? Is that just an artifact of the old algorithm? Doesn't that
>>>>>> introduce extra edges? What does this comment mean?
>>>>>> // Field node is created for all field types. It will help in
>>>>>> // split_unique_types(). Note, there will be no uses of non oop fields
>>>>>> // in Connection Graph.
>>>>>> int offset = address_offset(n, igvn);
>>>>>> add_field(n, PointsToNode::NoEscape, offset);
>>>>> I will change that comment:
>>>>>
>>>>> // Field nodes are created for all field types. They are used in
>>>>> // adjust_scalar_replaceable_state() and split_unique_types().
>>>>> // Note, non-oop fields will have only base edges in Connection
>>>>> // Graph because such fields are not used for oop loads and stores.
>>>> Ok.
>>>>
>>>>> Such fields are used in adjust_scalar_replaceable_state() for next checks:
>>>>>
>>>>> // 3. An object is not scalar replaceable if it has a field with unknown
>>>>> // offset (array's element is accessed in loop).
>>>>>
>>>>> // 5. Or the address may point to more then one object. This may produce
>>>>>
>>>>>> Is that to ensure that type information for the slice is created?
>>>>> Yes. I need to make sure that all related AddP nodes are processed in split_unique_types().
>>>>>
>>>>>> escape.hpp:
>>>>>> Do we really have to use uint instead of int? I have dreams of eliminating it from Node someday.
>>>>> I mostly use them to avoid C++ warning about comparing singed and unsigned which usually show up during JPRT build on
>>>>> windows. I replaced almost all uint to int, will see what will happened in JPRT.
>>>> I'd like to stamp out uint instead of spreading it's use. Should we turn on -Wsign-conversion in gcc so that it's
>>>> possible to test for some of the sign issues without having to do a windows build? I guess that could make it all worse.
>>>>
>>>>>> Is PointsToList really needed? Can't your just use GrowableArray and append_if_missing? You could modify it to
>>>>>> return a bool for your use.
>>>>> Originally PointsToList had mode methods but currently it is not needed, as you said. I removed PointsToList and
>>>>> modified GrowableArray::append_if_missing().
>>>> Cool.
>>>>
>>>>>> const Node* _node;
>>>>>> Don't you mean:
>>>>>> Node* const _node;
>>>>> No. I want this field to be set by constructor only and not modified after that.
>>>> That's the second one. The first is a pointer to a const Node and you want a const pointer to a Node. It binds to the
>>>> left unless there's a type name to the right.
>>>>
>>>>>> // Return true if nodes points only to non-escaped allocations.
>>>>>> bool not_escaped_allocation();
>>>>>> non escaping is used in other places and would read better here.
>>>>> Changed as suggested.
>>>>>
>>>>>> Why does process_call_arguments take the PhaseGVN as an argument?
>>>>> To get precise type of call's arguments:
>>>>>
>>>>> Node *arg = call->in(i);
>>>>> const Type *aat = igvn->type(arg);
>>>> But isn't it available as _igvn?
>>>>
>>>>>> escape.cpp:
>>>>>> Can you put the brace on the same line in this idiom to match other parts of C2:
>>>>>> case Op_DecodeN:
>>>>>> {
>>>>> Done.
>>>>>
>>>>>> Could you change this idiom:
>>>>>> if () {
>>>>>> ELSE_FAILED("foo");
>>>>>> }
>>>>>> break;
>>>>>> to something like:
>>>>>> if () {
>>>>>> break;
>>>>>> }
>>>>>> FAILED("foo");
>>>>>> elses inside #defines are hard to handle.
>>>>> Done.
>>>>>
>>>>>> These bools appear unused:
>>>>>> if (at->isa_oopptr() != NULL &&
>>>>>> arg_ptn->escape_state() < PointsToNode::GlobalEscape) {
>>>>>> bool global_escapes = false;
>>>>>> bool fields_escapes = false;
>>>>> Removed.
>>>>>
>>>>>> It might be nice to put something in the log if build_connection_graph bails out in product mode.
>>>>> Done:
>>>>> <connectionGraph_bailout reason='reached iterations limit'/>
>>>>>
>>>>>> typos:
>>>>>> // Check if a oop field's
>>>>>> // Non escaped object should points only to fields.
>>>>> Fixed.
>>>> Thanks!
>>>>
>>>> tom
>>>>
>>>>> I will submit JPRT test job to see how it goes.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>> On Mar 5, 2012, at 4:03 PM, Vladimir Kozlov wrote:
>>>>>>> http://cr.openjdk.java.net/~kvn/7147744/webrev.2
>>>>>>>
>>>>>>> Rearranged code in escape.cpp to move compute_escape() and related methods to the beginning of file. Moved parts of
>>>>>>> code in compute_escape() into separate methods: complete_connection_graph(), verify_connection_graph(),
>>>>>>> optimize_ideal_graph(). Split build_connection_graph() into 2 methods: add_node_to_connection_graph() and
>>>>>>> add_final_edges().
>>>>>>>
>>>>>>> Replaced next patterns:
>>>>>>> uint ecnt = use->edge_count();
>>>>>>> for (uint j = 0; j < ecnt; j++) {
>>>>>>> PointsToNode* e = use->edge(j);
>>>>>>>
>>>>>>> with simple iterators (EdgeIterator, UseIterator, BaseIterator):
>>>>>>>
>>>>>>> for (EdgeIterator i(use); i.has_next(); i.next()) {
>>>>>>> PointsToNode* e = i.get();
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> Tom Rodriguez wrote:
>>>>>>>> On Mar 2, 2012, at 2:42 PM, Vladimir Kozlov wrote:
>>>>>>>>> Thank you, Tom
>>>>>>>>>
>>>>>>>>> Tom Rodriguez wrote:
>>>>>>>>>> Are you preparing a new webrev for this issue?
>>>>>>>>> No. Last update I have is based on Christian's comments and it is mostly the same as original:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~kvn/7147744/webrev.1
>>>>>>>>>
>>>>>>>>> And I waited your comments to do final version.
>>>>>>>>>
>>>>>>>>>> I don't have anything too concrete to say. It looks ok and I think the structure seems a big nicer.
>>>>>>>>>> compute_escape desperately needs to be broken into multiple functions, particularly the verify code. It's very
>>>>>>>>>> hard to see the core structure.
>>>>>>>>> I do want it to be easy to understand so I will try to break it to several functions.
>>>>>>>> The big comments describing the different steps are helpful but they are so far apart it's hard to hold onto the
>>>>>>>> whole structure.
>>>>>>>> Part of the problem is that writing C2 code is somewhat verbose. These patterns for instance:
>>>>>>>> uint ucnt = base->use_count();
>>>>>>>> for (uint j = 0; j < ucnt; j++) {
>>>>>>>> PointsToNode* arycp = base->use(j);
>>>>>>>> might be done more compactly with an iterator similar to SimpleDUIterator. I've often though it would be useful to
>>>>>>>> have an iterator with a predicate built in so you could say things like:
>>>>>>>> for (SimpleDUIterator<is_ArrayCopy> i(n); i.has_next(); i.next()) {
>>>>>>>> ..
>>>>>>>> }
>>>>>>>> to concisely visit particular node types.
>>>>>>>> tom
>>>>>>>>> I may also rearrange it since I don't like that main method compute_escape() is somewhere in the middle of the
>>>>>>>>> source file.
>>>>>>>>>
>>>>>>>>>> build_connection_graph feels like it should be broken into two functions or at least that final switch should.
>>>>>>>>>> All the if (first_iteration) tests make it hard to follow since the else side if almost always empty.
>>>>>>>>> OK. I will look on it.
>>>>>>>>>
>>>>>>>>>> There are a lot of odd empty lines that just seems to spread the code out instead of separating logical things.
>>>>>>>>>> I'm not a huge fan of vertical white space. Anyway, nothing above is something that needs to be fixed for this
>>>>>>>>>> bug fix.
>>>>>>>>> I will try to clean up some.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Vladimir
>>>>>>>>>
>>>>>>>>>> tom
>>>>>>>>>> On Feb 29, 2012, at 12:58 PM, Vladimir Kozlov wrote:
>>>>>>>>>>> I rerun tests again and see difference because of 7146442 fix and not these changes. I have to look again on
>>>>>>>>>>> code in find_init_values() and may be do loads/stores domination checks.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Vladimir
>>>>>>>>>>>
>>>>>>>>>>> Vladimir Kozlov wrote:
>>>>>>>>>>>> Yes, I verified with refworkload benchmarks and EA tests I have. I compared PrintEliminateAllocations and
>>>>>>>>>>>> PrintEliminateLocks output.
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Vladimir
>>>>>>>>>>>> Tom Rodriguez wrote:
>>>>>>>>>>>>> I'm looking at it but it's pretty much a complete rewrite so it's hard to review. At a certain level it's
>>>>>>>>>>>>> going to mostly be a rubber stamp. Did you verify that it produces the same results as the old algorithm?
>>>>>>>>>>>>>
>>>>>>>>>>>>> tom
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Feb 28, 2012, at 2:01 PM, Vladimir Kozlov wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Please, I need a review for this.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Vladimir
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Vladimir Kozlov wrote:
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~kvn/7147744/webrev
>>>>>>>>>>>>>>> 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
>