Introduce phantom weak handles in the API and use them internally for debug info

Introduce phantom weak handles in the API and use them internally for debug info

R=jochen@chromium.org, ulan@chromium.org
BUG=

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

[Erik Corry Chromium.org, 2014-10-17 14:39:01]

erikcorry@chromium.org changed reviewers:
+ hpayer@chromium.org, jochen@chromium.org
- eisinger@chromium.org, hannes@chromium.org

[Erik Corry Chromium.org, 2014-10-20 08:53:24]

erikcorry@chromium.org changed reviewers:
+ ulan@chromium.org
- hpayer@chromium.org

[jochen (gone - plz use gerrit), 2014-10-20 13:51:14]

can we change it such that the phantom callback does not have to clear the
persistent, but just clear it internally anyways?

https://codereview.chromium.org/649563006/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/649563006/diff/1/src/compiler.cc#newcode1286
src/compiler.cc:1286: // the inner function must be allowed to be compiled
lazily.
nit. not related to this cl

https://codereview.chromium.org/649563006/diff/1/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/649563006/diff/1/src/globals.h#newcode294
src/globals.h:294: const uint64_t kPhantomReferenceZap = 0xca11bac;
should there be a 32bit and a 64bit zap value?

[Erik Corry, 2014-10-20 13:56:36]

erik.corry@gmail.com changed reviewers:
+ erik.corry@gmail.com

[Erik Corry, 2014-10-20 13:56:37]

I'll make the change so it does not need to be cleared.

https://codereview.chromium.org/649563006/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/649563006/diff/1/src/compiler.cc#newcode1286
src/compiler.cc:1286: // the inner function must be allowed to be compiled
lazily.
On 2014/10/20 13:51:14, jochen wrote:
> nit. not related to this cl

Does this mean you would rather leave the spelling error in?

https://codereview.chromium.org/649563006/diff/1/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/649563006/diff/1/src/globals.h#newcode294
src/globals.h:294: const uint64_t kPhantomReferenceZap = 0xca11bac;
On 2014/10/20 13:51:14, jochen wrote:
> should there be a 32bit and a 64bit zap value?

I think 32 bit is enough - it's converted to a Smi.

[jochen (gone - plz use gerrit), 2014-10-20 13:58:20]

if it compiles on windows, i'm fine with the zap value.

ideally, the comment fixes should go to a different cl, but I don't really care,
so lgtm either way

[ulan, 2014-10-20 14:03:11]

https://codereview.chromium.org/649563006/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/649563006/diff/20001/include/v8.h#newcode520
include/v8.h:520: V8_INLINE void SetPhantom(P* parameter,
Shouldn't phantom reference be in its own class, separate from PersistentBase to
completely disallow dereferencing?

What prevents the embedder stash a phantom reference p and then just dereference
it in the finalizer like this:
Local<Object> obj = Local<Object>::New(isolate, p)?
and access a potentialy dead object (*obj)

[jochen (gone - plz use gerrit), 2014-10-20 14:11:46]

https://codereview.chromium.org/649563006/diff/20001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/649563006/diff/20001/src/global-handles.cc#ne...
src/global-handles.cc:256: DCHECK(*handle ==
Smi::FromInt(kPhantomReferenceZap));
should we set handle to a null-handle here?

[Erik Corry, 2014-10-20 14:25:32]

https://codereview.chromium.org/649563006/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/649563006/diff/20001/include/v8.h#newcode520
include/v8.h:520: V8_INLINE void SetPhantom(P* parameter,
On 2014/10/20 14:03:10, ulan wrote:
> Shouldn't phantom reference be in its own class, separate from PersistentBase
to
> completely disallow dereferencing?

I think you want to allow deref before the object dies, otherwise I don't think
it is very useful.
 
> What prevents the embedder stash a phantom reference p and then just
dereference
> it in the finalizer like this:
> Local<Object> obj = Local<Object>::New(isolate, p)?
> and access a potentialy dead object (*obj)

I think nothing prevents this, but you can do that with normal weak references
too (not in the finalizer, but after resetting the persistent/global).  But
because it is zapped, it is not until the location is reused for something else
or freed up entirely that you can really run into problems - during the
finalizer you will get the same zapped value that the persistent looks at - just
a Smi.

https://codereview.chromium.org/649563006/diff/20001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/649563006/diff/20001/src/global-handles.cc#ne...
src/global-handles.cc:256: DCHECK(*handle ==
Smi::FromInt(kPhantomReferenceZap));
On 2014/10/20 14:11:46, jochen wrote:
> should we set handle to a null-handle here?

I think that's not so useful.  The callback then doesn't have an indication of
which handle is being cleared. By just zapping the contents, the callback still
has the location to key on if it has a hash map or similar. This is the value
the callback gets by doing operator* on the handle.

There is the parameter, but often that is used to identify the per-isolate
object that holds state, not the individual reference that is dying.  We could
add a second parameter, but that seems over-engineered somehow.

[ulan, 2014-10-20 14:26:02]

https://codereview.chromium.org/649563006/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/649563006/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:7492: 
Could you please also test we don't mark through a dying phantom handle?
I mean that phantom handle and its value die in the same GC.

[jochen (gone - plz use gerrit), 2014-10-20 14:29:04]

https://codereview.chromium.org/649563006/diff/20001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/649563006/diff/20001/src/global-handles.cc#ne...
src/global-handles.cc:256: DCHECK(*handle ==
Smi::FromInt(kPhantomReferenceZap));
On 2014/10/20 at 14:25:32, Erik Corry wrote:
> On 2014/10/20 14:11:46, jochen wrote:
> > should we set handle to a null-handle here?
> 
> I think that's not so useful.  The callback then doesn't have an indication of
which handle is being cleared. By just zapping the contents, the callback still
has the location to key on if it has a hash map or similar. This is the value
the callback gets by doing operator* on the handle.
> 
> There is the parameter, but often that is used to identify the per-isolate
object that holds state, not the individual reference that is dying.  We could
add a second parameter, but that seems over-engineered somehow.

but the address of the handle is completely new, it's allocated in the handle
scope two lines above. it's a fresh address, and it points to garbage.

the embedder will have to put all infos in the params anyways

[Erik Corry, 2014-10-22 13:21:01]

https://codereview.chromium.org/649563006/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/649563006/diff/20001/include/v8.h#newcode520
include/v8.h:520: V8_INLINE void SetPhantom(P* parameter,
I'll revise my answer to the second part slightly: If you do:

Local<Object> obj = Local<Object>::New(isolate, p)

then you get a new handle, which will keep alive the object, and the callback
will never be called.

On the other hand, internally you can do:

Handle<Object> handle(global_handle.location())

and this gives you a handle that points at the same location as the global
handle (persistent in external terminology).  This can be abused, but this is
not through the API.

https://codereview.chromium.org/649563006/diff/20001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/649563006/diff/20001/src/global-handles.cc#ne...
src/global-handles.cc:256: DCHECK(*handle ==
Smi::FromInt(kPhantomReferenceZap));
On 2014/10/20 14:29:03, jochen wrote:
> On 2014/10/20 at 14:25:32, Erik Corry wrote:
> > On 2014/10/20 14:11:46, jochen wrote:
> > > should we set handle to a null-handle here?
> > 
> > I think that's not so useful.  The callback then doesn't have an indication
of
> which handle is being cleared. By just zapping the contents, the callback
still
> has the location to key on if it has a hash map or similar. This is the value
> the callback gets by doing operator* on the handle.
> > 
> > There is the parameter, but often that is used to identify the per-isolate
> object that holds state, not the individual reference that is dying.  We could
> add a second parameter, but that seems over-engineered somehow.
> 
> but the address of the handle is completely new, it's allocated in the handle
> scope two lines above. it's a fresh address, and it points to garbage.
> 
> the embedder will have to put all infos in the params anyways

We could replace

Handle<Object> handle(*object, isolate)

with

Handle<Object> handle(object)

and then it would not be a new fresh address, but share the location.

However although that gives access to the location in the callback, the API user
can't get at the location when registering, because we don't let them get at the
location that backs a persistent (only a local).  I think there are good reasons
for that, so I'll go with your suggestion for now (API user can't use location,
has to use the parameter), and see how usable that is.

https://codereview.chromium.org/649563006/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/649563006/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:7492: 
On 2014/10/20 14:26:02, ulan wrote:
> Could you please also test we don't mark through a dying phantom handle?
> I mean that phantom handle and its value die in the same GC.

I haven't done this yet.

[jochen (gone - plz use gerrit), 2014-10-22 13:54:11]

i like it.

should we document that there won't be a handle scope for phantom callbacks?

[Erik Corry, 2014-10-22 16:15:04]

I think I'll just add the handle scope instead.

[Erik Corry, 2014-10-23 10:18:12]

PTAL

[jochen (gone - plz use gerrit), 2014-10-23 13:44:17]

lgtm, thanks!

[ulan, 2014-10-23 13:48:01]

lgtm

[Erik Corry Chromium.org, 2014-10-27 11:04:45]

Committed patchset #7 (id:120001) manually as 24899 (presubmit successful).