Don't overload FreeListElement for become.

Don't overload FreeListElement for become.

R=asiva@google.com, johnmccutchan@google.com

Committed: https://github.com/dart-lang/sdk/commit/3f8c0bb0a909066b3856f000ae7cbe17676369ff

[rmacnak, 2016-05-31 23:52:44]

rmacnak@google.com changed reviewers:
+ asiva@google.com, johnmccutchan@google.com

[siva, 2016-06-02 00:23:36]

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/become.h
File runtime/vm/become.h (right):

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/becom...
runtime/vm/become.h:37: // Used to allocate class for free list elements in
Object::InitOnce.
"free list elements" => "forwarding corpse elements"

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/becom...
runtime/vm/become.h:55: RawObject* target_;
Why did you do it this way instead of adding a RawForwardingCorpse class that
subclasses RawObject ?

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/objec...
runtime/vm/object.h:8522: // Free-list elements cannot be wrapped in a handle.
Update comment (Free List and Forwarding corpse) elements

[siva, 2016-06-02 00:27:57]

lgtm

[Cutch, 2016-06-02 14:09:53]

LGTM with a bunch of small comments

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/become.h
File runtime/vm/become.h (right):

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/becom...
runtime/vm/become.h:16: // corpses pointing to the right-hand side. Compare the
representation of a
you talk about "right-hand" side in the comment then use "target" in the API.
Maybe consistently use "source" and "target"?

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/becom...
runtime/vm/become.h:17: // FreeListElement.
I don't understand the last sentence.

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/class...
File runtime/vm/class_table.cc (right):

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/class...
runtime/vm/class_table.cc:222: if (i == kFreeListElement || i ==
kForwardingCorpse) {
This continue shouldn't be necessary anymore because we can now call Name on
both free list and forwarding corpse classes?

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/isola...
File runtime/vm/isolate_reload.cc (right):

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:975: if (obj->IsFreeListElement() ||
obj->IsForwardingCorpse()) {
This seems like a common enough pattern that we might want a utility method.

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/raw_o...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/raw_o...
runtime/vm/raw_object.h:187: kForwardingCorpse,
Add a comment about kForwardingCorpse

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/scave...
File runtime/vm/scavenger.cc (right):

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/scave...
runtime/vm/scavenger.cc:468: // The left-hand side of a become was a remembered
object, but we do
Was the TODO stale?

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/servi...
File runtime/vm/service.cc (right):

https://chromiumcodereview.appspot.com/2026643004/diff/20001/runtime/vm/servi...
runtime/vm/service.cc:3345: if (obj->IsFreeListElement() ||
obj->IsForwardingCorpse()) {
I really think a utility method might be helpful: 

bool IsFakeObject() const;

?

[rmacnak, 2016-06-02 17:57:07]

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/become.h
File runtime/vm/become.h (right):

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/become.h#new...
runtime/vm/become.h:16: // corpses pointing to the right-hand side. Compare the
representation of a
On 2016/06/02 14:09:53, Cutch wrote:
> you talk about "right-hand" side in the comment then use "target" in the API.
> Maybe consistently use "source" and "target"?

Changed to 

Objects that are a source in a become are tranformed into forwarding            
                                                                                
                 
corpses pointing to the corresponding target. Forwarding corpses have the       
                                                                                
                 
same heap sizes as the source object to ensure the heap remains walkable.       
                                                                                
                 
If the heap sizes is small enough to be encoded in the size field of the        
                                                                                
                 
header, a forwarding corpse consists only of a header and the target pointer.   
                                                                                
                 
If the heap size is too big to be encoded in the header's size field, the       
                                                                                
                 
word after the target pointer contains the size.  This is the same              
                                                                                
                 
representation as a FreeListElement.

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/become.h#new...
runtime/vm/become.h:37: // Used to allocate class for free list elements in
Object::InitOnce.
On 2016/06/02 00:23:36, siva wrote:
> "free list elements" => "forwarding corpse elements"

Done.

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/become.h#new...
runtime/vm/become.h:55: RawObject* target_;
On 2016/06/02 00:23:36, siva wrote:
> Why did you do it this way instead of adding a RawForwardingCorpse class that
> subclasses RawObject ?

As discussed offline, this follows the precedent of FreeListElement, the other
heap object that should never be reachable from a live object.

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/class_table.cc
File runtime/vm/class_table.cc (right):

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/class_table....
runtime/vm/class_table.cc:222: if (i == kFreeListElement || i ==
kForwardingCorpse) {
On 2016/06/02 14:09:53, Cutch wrote:
> This continue shouldn't be necessary anymore because we can now call Name on
> both free list and forwarding corpse classes?

Done.

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/isolate_relo...
File runtime/vm/isolate_reload.cc (right):

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:975: if (obj->IsFreeListElement() ||
obj->IsForwardingCorpse()) {
On 2016/06/02 14:09:53, Cutch wrote:
> This seems like a common enough pattern that we might want a utility method.

Done.

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:187: kForwardingCorpse,
On 2016/06/02 14:09:53, Cutch wrote:
> Add a comment about kForwardingCorpse

Done.

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:468: // The left-hand side of a become was a remembered
object, but we do
On 2016/06/02 14:09:53, Cutch wrote:
> Was the TODO stale?

The use of StorePointer during forwarding satisfies the following assert. The
rest of the todo was a bit confused because "remembered-ness" is not a property
that is forwarded.

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/2026643004/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:3345: if (obj->IsFreeListElement() ||
obj->IsForwardingCorpse()) {
On 2016/06/02 14:09:53, Cutch wrote:
> I really think a utility method might be helpful: 
> 
> bool IsFakeObject() const;
> 
> ?

Done.

[rmacnak, 2016-06-02 17:57:38]

Description was changed from

==========
Don't overload FreeListElement for become.
==========

to

==========
Don't overload FreeListElement for become.

R=asiva@google.com, johnmccutchan@google.com

Committed:
https://github.com/dart-lang/sdk/commit/3f8c0bb0a909066b3856f000ae7cbe17676369ff
==========

[rmacnak, 2016-06-02 17:57:40]

Committed patchset #3 (id:40001) manually as
3f8c0bb0a909066b3856f000ae7cbe17676369ff (presubmit successful).