Ignore deleted handles when processing weak roots.

Ignore deleted handles when processing weak roots.

Previously, deleted weak persistent handles in live handle blocks were
always assumed live.  With this change, dead handles are always ignored.

When a handle is deleted NULL is stored into the raw object member. This
looks like a Smi encoded 0.  As the handle blocks do not otherwise encode
which elements are live in an allocated block, the garbage collector now
uses this signal to distinguish live handles from deleted handles.

BUG=1640
Committed: https://code.google.com/p/dart/source/detail?r=4291

[siva, 2012-02-15 01:45:07]

LGTM

https://chromiumcodereview.appspot.com/9398014/diff/1/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://chromiumcodereview.appspot.com/9398014/diff/1/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:201: // Deleted handles are identified by a Smi as the
raw object value.
We could probably do a Clear() in FreeHandle() that way we would have a
Object::null() in raw_ and the comment would not be needed.

The check here is still valid as you rightly pointed out about the case when the
user sticks a Smi value here.

[cshapiro, 2012-02-15 03:18:47]

I figured out why I chose to stuff a Smi in the raw_ slot.  Consider the
following conditional which guards the finalize call

    if (!raw_obj->IsMarked() && raw_obj->IsOldObject()) {
      WeakPersistentHandle::Finalize(handle);
    }

Object::null() will answer "true" to IsOldObject().  As such, the finalize call
will be invoked.

I get a strange crash when I put Clear into the FreeHandle call.  I'll look into
this more in the morning.  It's probably something trivial.

[cshapiro, 2012-02-15 07:35:30]

It is trivial...  After a weak persistent handle is freed, the callback pointer
is used for the free list next link and so the WeakPersistnetHandle::Finalize
call will attempt to call the address of the next element in the free list
thinking it is a valid free pointer.

The obvious refinement is to use the peer field to thread the free list, and
then not clear the peer field when a handle is finalized.  In the short term
this would seem like a way out.  But, looking a few steps ahead this does not
seem so hot.  Another hack would be to steal a bit from the bottom of the
callback pointer.  That seems less attractive overall.

There is a subtle advantage to the Smi hack.  If we reduce strong persistent
handles to a 1 word structure, the raw_pointer field will be the only member
and, correspondingly, the only field through which the free list can be
threaded.  In that world, the free list links will have either a NULL value (end
of list) or a C pointer which otherwise looks like a Smi.  This motivates the
direction taken, even though the raw pointer field is left doing double duty as
an on-heap and off-heap pointer.

So, I propose going ahead with the code pretty much as-is to squash the bug at
hand.  Then, in a follow up change, switch over free list management code to
using the raw pointer field.

[siva, 2012-02-15 18:49:25]

> There is a subtle advantage to the Smi hack.  If we reduce strong persistent
> handles to a 1 word structure, the raw_pointer field will be the only member
> and, correspondingly, the only field through which the free list can be
> threaded.  In that world, the free list links will have either a NULL value
(end
> of list) or a C pointer which otherwise looks like a Smi.  This motivates the
> direction taken, even though the raw pointer field is left doing double duty
as
> an on-heap and off-heap pointer.


It seems tacky to assume that a C pointer will look like a Smi because all C
pointers are aligned. I would prefer not to make that assumption. Threading the
free list through the peer field seems safer. I see that we are already
overloading the raw_ field with a C pointer in the case of persistent handles.
We should at least add an assertion in SetNext that 'free_list' is aligned.


I am fine with using the Smi hack to squash this bug now and then revisiting
this in a follow up change.

[cshapiro, 2012-02-15 20:55:48]

> It seems tacky to assume that a C pointer will look like a Smi because all C
> pointers are aligned. I would prefer not to make that assumption. Threading
the
> free list through the peer field seems safer. I see that we are already
> overloading the raw_ field with a C pointer in the case of persistent handles.
> We should at least add an assertion in SetNext that 'free_list' is aligned.

The half-step of storing NULL into the raw object field is safe in this regard
as 0 is quite conveniently aligned.  But, we will have the same problem that the
regular persistent handle has if we re-use the raw field.

> I am fine with using the Smi hack to squash this bug now and then revisiting
> this in a follow up change.

Sounds good.