two pass phantom collection

two pass phantom collection

R=jochen@chromium.org, erikcorry@chromium.org

BUG=

Committed: https://crrev.com/2455aadf7b88f3da8e1f260238df40fbc768c208
Cr-Commit-Position: refs/heads/master@{#27475}

[dcarney, 2015-03-26 09:42:01]

dcarney@chromium.org changed reviewers:
+ erikcorry@chromium.org, jochen@chromium.org

[dcarney, 2015-03-26 09:42:08]

ptal

[jochen (gone - plz use gerrit), 2015-03-26 09:49:39]

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

https://codereview.chromium.org/998253006/diff/20001/src/global-handles.h#new...
src/global-handles.h:368: void* internal_fields_[2];
also use the constant here?

[Erik Corry, 2015-03-26 10:07:10]

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

[Erik Corry, 2015-03-26 10:07:11]

https://codereview.chromium.org/998253006/diff/60001/include/v8-util.h
File include/v8-util.h (right):

https://codereview.chromium.org/998253006/diff/60001/include/v8-util.h#newcod...
include/v8-util.h:328: p.Reset();
Why do we have to create a Global, then immediately reset it?

https://codereview.chromium.org/998253006/diff/60001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/998253006/diff/60001/include/v8.h#newcode506
include/v8.h:506: // Setting this to nullptr is unecessary and means there will
be no second
Comment is a bit confusing.  How about:

The default is nullptr, which means there will be no second pass.

Also, since this is part of V8's official API I think a bit more explanation is
in order.  As I understand it, this structure is passed to the first callback,
and the embedder can arrange for a second callback.  When is the second callback
called?

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

https://codereview.chromium.org/998253006/diff/60001/src/global-handles.cc#ne...
src/global-handles.cc:288: *location() = reinterpret_cast<Object*>(0x1);
I would prefer some zap value that you can grep for when you find it in the
debugger.  How about something phantom-themed:

0x6057ca11 // Gost-call

[dcarney, 2015-03-26 10:26:57]

https://codereview.chromium.org/998253006/diff/60001/include/v8-util.h
File include/v8-util.h (right):

https://codereview.chromium.org/998253006/diff/60001/include/v8-util.h#newcod...
include/v8-util.h:328: p.Reset();
On 2015/03/26 10:07:10, Erik Corry wrote:
> Why do we have to create a Global, then immediately reset it?

here nothing is being created.  we're just releasing the cell

https://codereview.chromium.org/998253006/diff/60001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/998253006/diff/60001/include/v8.h#newcode506
include/v8.h:506: // Setting this to nullptr is unecessary and means there will
be no second
On 2015/03/26 10:07:10, Erik Corry wrote:
> Comment is a bit confusing.  How about:
> 
> The default is nullptr, which means there will be no second pass.
> 
> Also, since this is part of V8's official API I think a bit more explanation
is
> in order.  As I understand it, this structure is passed to the first callback,
> and the embedder can arrange for a second callback.  When is the second
callback
> called?

Done.

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

https://codereview.chromium.org/998253006/diff/60001/src/global-handles.cc#ne...
src/global-handles.cc:288: *location() = reinterpret_cast<Object*>(0x1);
On 2015/03/26 10:07:11, Erik Corry wrote:
> I would prefer some zap value that you can grep for when you find it in the
> debugger.  How about something phantom-themed:
> 
> 0x6057ca11 // Gost-call

Done.

[dcarney, 2015-03-26 11:29:25]

ok, looking better. please take another look

[Erik Corry, 2015-03-26 11:32:11]

LGTM

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

https://codereview.chromium.org/998253006/diff/100001/test/cctest/test-api.cc...
test/cctest/test-api.cc:3200: CHECK_EQ(initial_handle_count,
global_handles->global_handles_count());
You can't assert this for an unthreaded test?

https://codereview.chromium.org/998253006/diff/120001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/998253006/diff/120001/include/v8.h#newcode508
include/v8.h:508: // calls may be called in the first callback. Should
additional work be
It would be great if we could enforce this.

[dcarney, 2015-03-26 11:37:15]

The CQ bit was checked by dcarney@chromium.org

[commit-bot: I haz the power, 2015-03-26 11:37:35]

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

[commit-bot: I haz the power, 2015-03-26 11:37:35]

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

[commit-bot: I haz the power, 2015-03-26 11:37:36]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Erik Corry Chromium.org, 2015-03-26 11:38:42]

lgtm

[dcarney, 2015-03-26 11:41:46]

The CQ bit was checked by dcarney@chromium.org

[commit-bot: I haz the power, 2015-03-26 11:42:06]

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

[commit-bot: I haz the power, 2015-03-26 11:50:24]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-26 11:50:42]

Patchset 8 (id:??) landed as
https://crrev.com/2455aadf7b88f3da8e1f260238df40fbc768c208
Cr-Commit-Position: refs/heads/master@{#27475}