Phantom references support internal fields

Phantom references support internal fields

BUG=

Committed: https://crrev.com/3ff951943f60ffbfe6c29325a9a9c39195de54b5
Cr-Commit-Position: refs/heads/master@{#25889}

[jochen (gone - plz use gerrit), 2014-11-21 16:33:17]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[rmcilroy, 2014-11-21 17:42:19]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[jochen (gone - plz use gerrit), 2014-11-24 12:27:53]

i think we need to copy out the internal fields when we iterate the weak roots
and zap the handle, no?

maybe declare a new (bigger) node type that is used for phantom handles?

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

https://codereview.chromium.org/753553002/diff/1/src/global-handles.cc#newcod...
src/global-handles.cc:635: *(node->location()) =
Smi::FromInt(kPhantomReferenceZap);
if you zap the handle here, how can you get to object() when you read out the
internal fields?

[Erik Corry, 2014-11-24 12:38:25]

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

[Erik Corry, 2014-11-24 12:38:26]

I might have to grow the node. That unfortunately means growing all the nodes,
since they are allocated in arrays and can't have individual sizes.

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

https://codereview.chromium.org/753553002/diff/1/src/global-handles.cc#newcod...
src/global-handles.cc:635: *(node->location()) =
Smi::FromInt(kPhantomReferenceZap);
On 2014/11/24 12:27:53, jochen (slow) wrote:
> if you zap the handle here, how can you get to object() when you read out the
> internal fields?

For the INTERNAL_FIELDS case we don't zap here.

[Erik Corry, 2014-11-24 12:39:23]

Not yet ready, still crashes on the young space case.

[jochen (gone - plz use gerrit), 2014-11-25 12:32:51]

On 2014/11/24 at 12:38:26, erik.corry wrote:
> I might have to grow the node. That unfortunately means growing all the nodes,
since they are allocated in arrays and can't have individual sizes.
We could have two node kinds

[picksi1, 2014-11-28 12:15:10]

picksi@chromium.org changed reviewers:
+ picksi@chromium.org

[picksi1, 2014-11-28 12:15:11]

Thanks for CC'ing me in on this! It's interesting stuff. To repay your efforts
I've made some possibly naive comments and questions (that'll teach you!).

https://codereview.chromium.org/753553002/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode516
include/v8.h:516: if (a == 0) return b == 0;
Should the '0's be NULL's for consistency with the [Is]Empty() functions above?

https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode517
include/v8.h:517: if (b == 0) return false;
If you changed the two above if's with...

if (a==b) return true; // both are null, or point at the same thing
if ((a & b)==0) return false; // One is null

... you'd save having to deference the pointer if a & b both point at the same
thing... Don't know if this is an impossible case though?

https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode6144
include/v8.h:6144: static const int kNodeStateMask = 0x7;
Can you generate this 0x7 mask? e.g. 

kNodeStateMask = kNodeStateIsWeakValue | kNodeStateIsPendingValue |
kNodeStateIsNearDeathValue

... Or are those the wrong consts?

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

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:102: void Zap() {
Is 'Zap' a known term in the world of GC? For the naive reader (i.e. me) it
doesn't tell me what the function does! Should your comment inside the function
be turned into the name? e.g. MarkForEagerTrapping()?

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:166: flags_ = NodeWeaknessType::update(flags_, t);
is t a good variable name here? should it be weakness_type?

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:263: if (parameter != NULL) {
Bikeshed: Might be clearer to swap the if/else bodies and turn the != into ==

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:301: if (weakness_type() == Node::NORMAL_WEAK) {
Could this code be refactored to switch on the weakness_type? It might be [very
slightly!] quicker than the cascading ifs, and possible clearer? Although you
may end up with duplicate phantom callback code in the PHANTOM_WEAK and
INTERNAL_FIELDS_WEAK case.

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:320: if (state() != FREE) {
There are lots of 'state() != FREE's in the code, is it worth creating a
function (state_not_free()?) that captures this concept?

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:586: Node::FromLocation(location)
Do internal_field_index1/2 have anything to do with the internal_field1/2
elements in the union+struct on line 388 above? They are named almost the same
but are different types, should they be "harmonised"?

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:635: if (node->state() == Node::PENDING) {
switch? Not sure if a 3 case switch beats 2 ifs on performance!

[Erik Corry, 2014-11-28 14:21:25]

https://codereview.chromium.org/753553002/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode516
include/v8.h:516: if (a == 0) return b == 0;
On 2014/11/28 12:15:11, picksi1 wrote:
> Should the '0's be NULL's for consistency with the [Is]Empty() functions
above?

Done.

https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode517
include/v8.h:517: if (b == 0) return false;
On 2014/11/28 12:15:11, picksi1 wrote:
> If you changed the two above if's with...
> 
> if (a==b) return true; // both are null, or point at the same thing
> if ((a & b)==0) return false; // One is null
> 
> ... you'd save having to deference the pointer if a & b both point at the same
> thing... Don't know if this is an impossible case though?

Not sure, but you definitely don't want bitwise & here :-)

https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode6144
include/v8.h:6144: static const int kNodeStateMask = 0x7;
On 2014/11/28 12:15:11, picksi1 wrote:
> Can you generate this 0x7 mask? e.g. 
> 
> kNodeStateMask = kNodeStateIsWeakValue | kNodeStateIsPendingValue |
> kNodeStateIsNearDeathValue
> 
> ... Or are those the wrong consts?

Could be, but I'm going to stay consistent with the style in the rest of the
file.

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

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:102: void Zap() {
On 2014/11/28 12:15:11, picksi1 wrote:
> Is 'Zap' a known term in the world of GC? For the naive reader (i.e. me) it
> doesn't tell me what the function does! Should your comment inside the
function
> be turned into the name? e.g. MarkForEagerTrapping()?

It's pretty well established, I think.

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:166: flags_ = NodeWeaknessType::update(flags_, t);
On 2014/11/28 12:15:11, picksi1 wrote:
> is t a good variable name here? should it be weakness_type?

Done.

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:263: if (parameter != NULL) {
On 2014/11/28 12:15:11, picksi1 wrote:
> Bikeshed: Might be clearer to swap the if/else bodies and turn the != into ==

Done.

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:301: if (weakness_type() == Node::NORMAL_WEAK) {
On 2014/11/28 12:15:11, picksi1 wrote:
> Could this code be refactored to switch on the weakness_type? It might be
[very
> slightly!] quicker than the cascading ifs, and possible clearer? Although you
> may end up with duplicate phantom callback code in the PHANTOM_WEAK and
> INTERNAL_FIELDS_WEAK case.

I have to rewrite this anyway because I have worked out why it's completely
wrong.

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:320: if (state() != FREE) {
On 2014/11/28 12:15:11, picksi1 wrote:
> There are lots of 'state() != FREE's in the code, is it worth creating a
> function (state_not_free()?) that captures this concept?

Created IsInUse() for this.

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:586: Node::FromLocation(location)
On 2014/11/28 12:15:11, picksi1 wrote:
> Do internal_field_index1/2 have anything to do with the internal_field1/2
> elements in the union+struct on line 388 above? They are named almost the same
> but are different types, should they be "harmonised"?

Done.

https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#ne...
src/global-handles.cc:635: if (node->state() == Node::PENDING) {
On 2014/11/28 12:15:11, picksi1 wrote:
> switch? Not sure if a 3 case switch beats 2 ifs on performance!

The compiler will certainly convert a 3 case switch into two 'if's.  I think the
ifs are more readable.

[picksi1, 2014-11-28 14:51:05]

https://codereview.chromium.org/753553002/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode517
include/v8.h:517: if (b == 0) return false;
On 2014/11/28 14:21:25, Erik Corry wrote:
> On 2014/11/28 12:15:11, picksi1 wrote:
> > If you changed the two above if's with...
> > 
> > if (a==b) return true; // both are null, or point at the same thing
> > if ((a & b)==0) return false; // One is null
> > 
> > ... you'd save having to deference the pointer if a & b both point at the
same
> > thing... Don't know if this is an impossible case though?
> 
> Not sure, but you definitely don't want bitwise & here :-)

:) Oops

[picksi1, 2014-11-28 14:58:19]

lgtm!

[jochen (gone - plz use gerrit), 2014-12-02 10:25:59]

https://codereview.chromium.org/753553002/diff/80001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/753553002/diff/80001/include/v8.h#newcode510
include/v8.h:510: V8_INLINE void Empty() { val_ = NULL; }
what do you need Empty() for?

https://codereview.chromium.org/753553002/diff/80001/include/v8.h#newcode565
include/v8.h:565: int internalFieldOffset1, int internalFieldOffset2);
internal_field_index1

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

https://codereview.chromium.org/753553002/diff/80001/src/global-handles.cc#ne...
src/global-handles.cc:43: INTERNAL_FIELDS_WEAK  // Embedded gets 2 internal
fields from dying object.
Embedder

https://codereview.chromium.org/753553002/diff/80001/src/global-handles.cc#ne...
src/global-handles.cc:272: set_internal_fields(internal_field_index1,
internal_field_index2);
is there a check somewhere that the object actually has that many internal
fields?

https://codereview.chromium.org/753553002/diff/80001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/753553002/diff/80001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2219: ProcessMarkingDeque();
this is not needed, because the following call to ProcessEphemeralMarking
processes the deque anyways

[Erik Corry, 2014-12-02 10:40:49]

I still need to fix creation and processing of the deferred phantom callback
queue in scavenges (works for mark-sweep).

https://codereview.chromium.org/753553002/diff/80001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/753553002/diff/80001/include/v8.h#newcode510
include/v8.h:510: V8_INLINE void Empty() { val_ = NULL; }
On 2014/12/02 10:25:59, jochen (slow) wrote:
> what do you need Empty() for?

It seems I don't need it any more.  Removed.

https://codereview.chromium.org/753553002/diff/80001/include/v8.h#newcode565
include/v8.h:565: int internalFieldOffset1, int internalFieldOffset2);
On 2014/12/02 10:25:59, jochen (slow) wrote:
> internal_field_index1

Done.

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

https://codereview.chromium.org/753553002/diff/80001/src/global-handles.cc#ne...
src/global-handles.cc:43: INTERNAL_FIELDS_WEAK  // Embedded gets 2 internal
fields from dying object.
On 2014/12/02 10:25:59, jochen (slow) wrote:
> Embedder

Done.

https://codereview.chromium.org/753553002/diff/80001/src/global-handles.cc#ne...
src/global-handles.cc:272: set_internal_fields(internal_field_index1,
internal_field_index2);
On 2014/12/02 10:25:59, jochen (slow) wrote:
> is there a check somewhere that the object actually has that many internal
> fields?

Yes, it's in InternalFieldOK, called by SlowGetInternalField, called by
GetInternalField (if checks are enabled).

https://codereview.chromium.org/753553002/diff/80001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/753553002/diff/80001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2219: ProcessMarkingDeque();
On 2014/12/02 10:25:59, jochen (slow) wrote:
> this is not needed, because the following call to ProcessEphemeralMarking
> processes the deque anyways

I think this is wrong, and it is needed.

Nothing in the ProcessEphemeralMarking will check for an overflowed marking
deque, before the work_to_do variable is set.  That means it can be incorrectly
set to false, and the ephemeral iteration can terminate early.

[rmcilroy, 2014-12-03 11:56:35]

https://codereview.chromium.org/753553002/diff/80001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/753553002/diff/80001/include/v8.h#newcode422
include/v8.h:422: class CallbackData {
nit - BaseCallbackData or maybe even BaseWeakCallbackData? 

Would it make sense to put this in a non-public namespace to make it obvious
this isn't intended to be used as part of the external API.

https://codereview.chromium.org/753553002/diff/80001/include/v8.h#newcode435
include/v8.h:435: 
nit - two newlines between classes in V8

https://codereview.chromium.org/753553002/diff/80001/include/v8.h#newcode463
include/v8.h:463: : CallbackData<T>(isolate, NULL),
Do we want to expose NULL parameters to PhantomCallbacks - it feels like that
could get error prone if the caller has to know whether to check if
GetParameter() returns null (maybe this is already the case though)?  Would it
make sense to have different callbacks for either the internal field version and
the parameter version?

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

https://codereview.chromium.org/753553002/diff/80001/src/global-handles.cc#ne...
src/global-handles.cc:662: // We stil need to visit the pointer in case the
object moved,
/s/stil/still

[Erik Corry, 2014-12-15 15:12:42]

https://codereview.chromium.org/753553002/diff/80001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/753553002/diff/80001/include/v8.h#newcode422
include/v8.h:422: class CallbackData {
On 2014/12/03 11:56:35, rmcilroy wrote:
> nit - BaseCallbackData or maybe even BaseWeakCallbackData? 
> 
> Would it make sense to put this in a non-public namespace to make it obvious
> this isn't intended to be used as part of the external API.

Done.

https://codereview.chromium.org/753553002/diff/80001/include/v8.h#newcode435
include/v8.h:435: 
On 2014/12/03 11:56:35, rmcilroy wrote:
> nit - two newlines between classes in V8

Done.

https://codereview.chromium.org/753553002/diff/80001/include/v8.h#newcode463
include/v8.h:463: : CallbackData<T>(isolate, NULL),
On 2014/12/03 11:56:35, rmcilroy wrote:
> Do we want to expose NULL parameters to PhantomCallbacks - it feels like that
> could get error prone if the caller has to know whether to check if
> GetParameter() returns null (maybe this is already the case though)?  Would it
> make sense to have different callbacks for either the internal field version
and
> the parameter version?

Fixed to pass a type that has only valid fields to the callback.

https://codereview.chromium.org/753553002/diff/80001/include/v8.h#newcode510
include/v8.h:510: V8_INLINE void Empty() { val_ = NULL; }
On 2014/12/02 10:40:48, Erik Corry wrote:
> On 2014/12/02 10:25:59, jochen (slow) wrote:
> > what do you need Empty() for?
> 
> It seems I don't need it any more.  Removed

Actually I need this for the corresponding Blink change.  In the callback for
the scriptwrappable we empty the handle, which has been zapped on the V8 side.
This means we don't try to touch it from the destructor later.

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

https://codereview.chromium.org/753553002/diff/80001/src/global-handles.cc#ne...
src/global-handles.cc:662: // We stil need to visit the pointer in case the
object moved,
On 2014/12/03 11:56:35, rmcilroy wrote:
> /s/stil/still

Done.

[Erik Corry, 2014-12-15 15:18:30]

This latest version queues up the callbacks and then calls them later.  We have
to do that because we can't in general do callbacks before allocation is allowed
again, but for scavenges that's too late to look at dying objects (they could
get overwritten).  But right now we don't postpone much (it's still in the same
atomic end-of-GC event for mark-sweep).

I don't plan to make any more big changes before committing, so please take a
look.

This is a prerequisite for https://codereview.chromium.org/806693004

[jochen (gone - plz use gerrit), 2014-12-15 15:39:40]

lgtm assuming you can make the compilers happy :)

[rmcilroy, 2014-12-16 10:59:07]

On 2014/12/15 15:39:40, jochen (slow) wrote:
> lgtm assuming you can make the compilers happy :)

lgtm, thanks.

[Erik Corry Chromium.org, 2014-12-16 15:04:19]

The CQ bit was checked by erikcorry@chromium.org

[commit-bot: I haz the power, 2014-12-16 15:05:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/753553002/260001

[commit-bot: I haz the power, 2014-12-16 15:52:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-16 15:52:41]

Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/486)

[Erik Corry Chromium.org, 2014-12-18 15:32:40]

The CQ bit was checked by erikcorry@chromium.org

[commit-bot: I haz the power, 2014-12-18 15:33:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/753553002/280002

[commit-bot: I haz the power, 2014-12-18 16:09:13]

Committed patchset #16 (id:280002)

[commit-bot: I haz the power, 2014-12-18 18:05:43]

Patchset 16 (id:??) landed as
https://crrev.com/3ff951943f60ffbfe6c29325a9a9c39195de54b5
Cr-Commit-Position: refs/heads/master@{#25889}

[dcarney, 2015-01-07 13:18:50]

dcarney@chromium.org changed reviewers:
+ dcarney@chromium.org

[dcarney, 2015-01-07 13:18:51]

https://codereview.chromium.org/753553002/diff/280002/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/753553002/diff/280002/include/v8.h#newcode477
include/v8.h:477: T* internal_field1_;
I'm a little late weighing in here, but I have a few points of concern.  The
first is that these two fields can be heap objects, which is crazily dangerous
for about 6 reasons.  We should set nullptr for any values that are not aligned
according to SetAlignedPointerInInternalField, which won't catch all smis
(probably we should box any smis set as internal fields).

The second problem is that in many cases in blink we want both the internal
fields and the parameter, so splitting into two callbacks types is a pretty bad
idea here.  I think setting a bit saying you need up to the first 2 internal
fields if they are there should be okay, since an embedder should be able to
rearrange things such that those fields are in the first two slots.  There
should be no additional memory required at that point for global handle nodes,
just additional memory for the phantom callback queue, which should be fine.

Lastly, these field are set with void*, so I think getting them with void*
should be okay, avoiding extra template parameters, but i'm pretty indifferent
to this.