External references should not affect dominance relation.

External references should not affect dominance relation.

Committed: https://code.google.com/p/v8/source/detail?r=11247

[alexeif, 2012-04-05 16:23:16]

Misha, could you please take a look.

[mnaganov (inactive), 2012-04-05 22:00:13]

Please add more explanations of the change to the commit description.

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator.cc
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator....
src/profile-generator.cc:2158: Object* value = target->IsJSGlobalPropertyCell()
What happens to JSGlobalPropertyCells in a snapshot now? Are they connected to
any node?

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator....
src/profile-generator.cc:2164: TagObject(value, "(hidden properties)");
Will there be only single entry with the name "hidden_properties"?

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator....
src/profile-generator.cc:3200: if (edge->from()->reachable_from_window()) return
true;
Why not:

return edge->from()->reachable_from_window() ||
!edge->to()->reachable_from_window();

?

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator.h
File src/profile-generator.h (right):

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator....
src/profile-generator.h:602: unsigned reachable_from_window_: 1;
Do we really need an additional field? It seems that 'painted_' bit is only used
in FillPostorderIndexes. We can first fill the indexes, and then mark reachable
objects, reusing the same bit.

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator....
src/profile-generator.h:1102: void FillPostorderIndexes(Vector<HeapEntry*>*
entries);
Why the rename? "Reverse" means that children will have order indexes in reverse
to their child index, as we are processing them backwards.

[alexeif, 2012-04-06 10:04:36]

PTAL

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator.cc
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator....
src/profile-generator.cc:2158: Object* value = target->IsJSGlobalPropertyCell()
On 2012/04/05 22:00:13, Mikhail Naganov (Chromium) wrote:
> What happens to JSGlobalPropertyCells in a snapshot now? Are they connected to
> any node?
They are still present in the object properties array.

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator....
src/profile-generator.cc:2164: TagObject(value, "(hidden properties)");
On 2012/04/05 22:00:13, Mikhail Naganov (Chromium) wrote:
> Will there be only single entry with the name "hidden_properties"?
I don't have a proof for it, but I have never seen two of them. Anyway instead
of entry(ies) with (empty) name we'll now have a named one.

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator....
src/profile-generator.cc:3200: if (edge->from()->reachable_from_window()) return
true;
On 2012/04/05 22:00:13, Mikhail Naganov (Chromium) wrote:
> Why not:
> 
> return edge->from()->reachable_from_window() ||
> !edge->to()->reachable_from_window();
> 
> ?
As you wish!

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator.h
File src/profile-generator.h (right):

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator....
src/profile-generator.h:602: unsigned reachable_from_window_: 1;
On 2012/04/05 22:00:13, Mikhail Naganov (Chromium) wrote:
> Do we really need an additional field? It seems that 'painted_' bit is only
used
> in FillPostorderIndexes. We can first fill the indexes, and then mark
reachable
> objects, reusing the same bit. 
They both are in use in FillPostorderIndexes.

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator....
src/profile-generator.h:1102: void FillPostorderIndexes(Vector<HeapEntry*>*
entries);
On 2012/04/05 22:00:13, Mikhail Naganov (Chromium) wrote:
> Why the rename? "Reverse" means that children will have order indexes in
reverse
> to their child index, as we are processing them backwards.
This algorithm assigns postorder indexes which means the parent is processed
after its nodes. AFAICT word "Reverse" came from the dominators building
algorithm where we're processing postorder sorted nodes in reverse order. So the
word reverse has no relation to this particular function, thus I removed it.

[alexeif, 2012-04-06 10:07:11]

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator.h
File src/profile-generator.h (right):

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator....
src/profile-generator.h:1102: void FillPostorderIndexes(Vector<HeapEntry*>*
entries);
On 2012/04/06 10:04:36, alexeif wrote:
> On 2012/04/05 22:00:13, Mikhail Naganov (Chromium) wrote:
> > Why the rename? "Reverse" means that children will have order indexes in
> reverse
> > to their child index, as we are processing them backwards.
> This algorithm assigns postorder indexes which means the parent is processed
> after its nodes. AFAICT word "Reverse" came from the dominators building
typo: read "after its children".
> algorithm where we're processing postorder sorted nodes in reverse order. So
the
> word reverse has no relation to this particular function, thus I removed it.

[mnaganov (inactive), 2012-04-06 14:16:01]

LGTM

Please add more details to the description of the change before committing.

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator.h
File src/profile-generator.h (right):

https://chromiumcodereview.appspot.com/10007009/diff/1/src/profile-generator....
src/profile-generator.h:602: unsigned reachable_from_window_: 1;
On 2012/04/06 10:04:36, alexeif wrote:
> On 2012/04/05 22:00:13, Mikhail Naganov (Chromium) wrote:
> > Do we really need an additional field? It seems that 'painted_' bit is only
> used
> > in FillPostorderIndexes. We can first fill the indexes, and then mark
> reachable
> > objects, reusing the same bit. 
> They both are in use in FillPostorderIndexes.

Sorry, didn't realize that. It is queried in IsRetainingEdge.