Provide API support for weak handles and post-mortem finalization.

Provide API support for weak handles and post-mortem finalization.

In addition, this change separates the storage of weak persistent
handles from ordinary persistent handles.  An ordinary persistent
handle now stores no other data than the raw pointer value.

Committed: https://code.google.com/p/dart/source/detail?r=3273

[Anton Muhin, 2012-01-12 13:40:26]

DBC

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/gc_marker.cc#newc...
runtime/vm/gc_marker.cc:202: handle->Finalize();
there might be unpleasant problem: naively implemented finalizer may reenter
into Dart and therefore into GC.  Chances are high you would like to prohibit
such behaviour, but that has some implications:

1) documentation should be updated;
2) you should probably provide some callbacks to clean up objects pending
destruction---if all finalizer can do is to move an object into some
to-be-deleted queue, embedder needs a trigger for it, ideally {s,}he should be
able to do that at the first safe point;
3) it might be worth adding a checks, at least in DEBUG mode, that verifies that
finalizer doesn't reenter into GC from finalizer.

Or you may allow finalizers to reenter into Dart (as v8 does).

[cshapiro, 2012-01-12 18:15:36]

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/gc_marker.cc#newc...
runtime/vm/gc_marker.cc:202: handle->Finalize();
Correct.  There is one case we consider to be safe and that is calling back into
the VM to delete a handle.  Aside from that, it is my intention to establish a
"no callback" scope in or around the invocation of Finalize() that will prohibit
all other callbacks into the VM.  At the very least this will be a debug mode
check as you suggested in #3.

Regarding #2, unlike V8, which signals when an object is about to be reclaimed,
Dart signals when an object is already dead.  As such, we cannot provide the
callback with access to the object as it no longer exists.  The intention is for
the embedding application to provide enough state in the peer to make a cleanup
action possible.

[Ivan Posva, 2012-01-12 23:30:38]

LGTM with comments.

-Ivan

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:513: state->persistent_handles().FreeHandle(ref);
Why not checking for protected handles?

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/handles.h
File runtime/vm/handles.h (right):

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/handles.h#newcode63
runtime/vm/handles.h:63: virtual void VisitHandle(uword* addr) = 0;
uword is already an address type, so please just use "VisitHandle(uword
handle)".

Also I can imagine that in the future we might need to pass a kind of weak
handle.

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/verifier.cc
File runtime/vm/verifier.cc (right):

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/verifier.cc#newco...
runtime/vm/verifier.cc:53: Isolate::Current()->VisitObjectPointers(&visitor,
Isolate* isolate = Isolate::Current();
isolate->Visit...

[cshapiro, 2012-01-13 01:30:02]

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:513: state->persistent_handles().FreeHandle(ref);
A good idea.  Done.  This makes the diff a bit more symmetric.

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/handles.h
File runtime/vm/handles.h (right):

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/handles.h#newcode63
runtime/vm/handles.h:63: virtual void VisitHandle(uword* addr) = 0;
Done (here and in the callers).

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/verifier.cc
File runtime/vm/verifier.cc (right):

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/verifier.cc#newco...
runtime/vm/verifier.cc:53: Isolate::Current()->VisitObjectPointers(&visitor,
Thanks, much cleaner.  Done.

[Anton Muhin, 2012-01-13 14:20:56]

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/gc_marker.cc#newc...
runtime/vm/gc_marker.cc:202: handle->Finalize();
Carl, sorry, I wasn't clear enough regarding #2.

Imagine the common case: one wraps a pointer into Dart object and wants to
destroy the objects once Dart wrapper is unreachable.  With the current design
one cannot just invoke the dtor on the pointer as typically, esp. in the big
projects, one cannot foresee if dtor will reenter Dart or not.  That means that
one has to defer dtor invocation.  On the other hand, the sooner object is
destroyed and the memory is reclaimed, the better.  Therefore, one option might
be a callback which is invoked precisely after GC happened---the embedder will
destroy all the queued objects in this callback.  Does that make sense?

On 2012/01/12 18:15:38, cshapiro wrote:
> Correct.  There is one case we consider to be safe and that is calling back
into
> the VM to delete a handle.  Aside from that, it is my intention to establish a
> "no callback" scope in or around the invocation of Finalize() that will
prohibit
> all other callbacks into the VM.  At the very least this will be a debug mode
> check as you suggested in #3.
> 
> Regarding #2, unlike V8, which signals when an object is about to be
reclaimed,
> Dart signals when an object is already dead.  As such, we cannot provide the
> callback with access to the object as it no longer exists.  The intention is
for
> the embedding application to provide enough state in the peer to make a
cleanup
> action possible.

[cshapiro, 2012-01-13 22:56:45]

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

http://codereview.chromium.org/9148051/diff/4001/runtime/vm/gc_marker.cc#newc...
runtime/vm/gc_marker.cc:202: handle->Finalize();
Thanks for the clarification.

I discussed your concern with Ivan and the current opinion is that situations
where the finalizer is non-trivial should be resolved by queuing an object for
destruction through the event queue (I believe the render even queue was
mentioned) and having those actions be handled outside the garbage collector.

If you feel this does not work for you, let's have a discussion outside this
review to resolve whether or not we have something workable.