[promises] Move async debug event creation to c++

[promises] Move async debug event creation to c++

BUG=v8:5343
Committed: https://crrev.com/18a116c7bdf1bcd6a3d6b9419bbc98fffd28fd45
Cr-Commit-Position: refs/heads/master@{#40369}

[gsathya, 2016-10-13 03:45:30]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 03:45:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 04:14:01]

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

[commit-bot: I haz the power, 2016-10-13 04:14:02]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-10-13 04:52:15]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 04:52:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2016-10-13 04:52:43]

Description was changed from

==========
[promises] Move async debug event creation to c++

BUG=v8:5343
==========

to

==========
[promises] Move async debug event creation to c++

BUG=v8:5343
==========

[gsathya, 2016-10-13 04:52:43]

gsathya@chromium.org changed reviewers:
+ adamk@chromium.org

[commit-bot: I haz the power, 2016-10-13 05:52:37]

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

[commit-bot: I haz the power, 2016-10-13 05:52:38]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-10-13 15:59:09]

adamk@chromium.org changed reviewers:
+ yangguo@chromium.org

[adamk, 2016-10-13 15:59:11]

+yangguo for debug/

https://codereview.chromium.org/2415023002/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2415023002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:1786: Handle<Object> name) {
And then you can add a DCHECK(id->IsNumber()) here.

https://codereview.chromium.org/2415023002/diff/20001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2415023002/diff/20001/src/debug/debug.h#newco...
src/debug/debug.h:420: void OnAsyncTaskEvent(Handle<Object> type, Handle<Object>
id,
This should be able to be more strictly typed: type and name are always Strings.

https://codereview.chromium.org/2415023002/diff/20001/src/debug/debug.h#newco...
src/debug/debug.h:592: MUST_USE_RESULT MaybeHandle<Object>
MakeAsyncTaskEvent(Handle<Object> type,
Same type comment here.

https://codereview.chromium.org/2415023002/diff/20001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2415023002/diff/20001/src/isolate.cc#newcode3081
src/isolate.cc:3081: static const char will_handle[] = "willHandle";
Rather than recreating a string for this every time, add this to the string list
in heap-symbols.h (same goes for didHandle).

https://codereview.chromium.org/2415023002/diff/20001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2415023002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:922: CHECK(id()->IsSmi() || id()->IsUndefined(isolate) ||
id()->IsObject());
What is the IsObject() case for (and how could that ever fail?)?

Also, I think IsNumber() is what you want instead of IsSmi() (it's hard to
guarantee that a number created in JS is a Smi).

https://codereview.chromium.org/2415023002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2415023002/diff/20001/src/objects.h#newcode6693
src/objects.h:6693: DECL_ACCESSORS(name, Object)
In this context, "id" and "name" are too generic. How about "debug_id" and
"debug_name"? Same goes for PromiseReactionJobInfo.

[gsathya, 2016-10-13 21:21:47]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 21:21:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2016-10-13 21:27:30]

https://codereview.chromium.org/2415023002/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2415023002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:1786: Handle<Object> name) {
On 2016/10/13 15:59:10, adamk wrote:
> And then you can add a DCHECK(id->IsNumber()) here.

Done.

https://codereview.chromium.org/2415023002/diff/20001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2415023002/diff/20001/src/debug/debug.h#newco...
src/debug/debug.h:420: void OnAsyncTaskEvent(Handle<Object> type, Handle<Object>
id,
On 2016/10/13 15:59:10, adamk wrote:
> This should be able to be more strictly typed: type and name are always
Strings.

Done.

https://codereview.chromium.org/2415023002/diff/20001/src/debug/debug.h#newco...
src/debug/debug.h:592: MUST_USE_RESULT MaybeHandle<Object>
MakeAsyncTaskEvent(Handle<Object> type,
On 2016/10/13 15:59:10, adamk wrote:
> Same type comment here.

Done.

https://codereview.chromium.org/2415023002/diff/20001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2415023002/diff/20001/src/isolate.cc#newcode3081
src/isolate.cc:3081: static const char will_handle[] = "willHandle";
On 2016/10/13 15:59:10, adamk wrote:
> Rather than recreating a string for this every time, add this to the string
list
> in heap-symbols.h (same goes for didHandle).

Done.

https://codereview.chromium.org/2415023002/diff/20001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2415023002/diff/20001/src/objects-debug.cc#ne...
src/objects-debug.cc:922: CHECK(id()->IsSmi() || id()->IsUndefined(isolate) ||
id()->IsObject());
On 2016/10/13 15:59:10, adamk wrote:
> What is the IsObject() case for (and how could that ever fail?)?

That was just a catch-all, since Smi isn't guaranteed as you mention below. I
didn't know about IsNumber.

> Also, I think IsNumber() is what you want instead of IsSmi() (it's hard to
> guarantee that a number created in JS is a Smi).

Done.

https://codereview.chromium.org/2415023002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2415023002/diff/20001/src/objects.h#newcode6693
src/objects.h:6693: DECL_ACCESSORS(name, Object)
On 2016/10/13 15:59:10, adamk wrote:
> In this context, "id" and "name" are too generic. How about "debug_id" and
> "debug_name"? Same goes for PromiseReactionJobInfo.

Agreed about id. Name is either Promise.resolve/reject, async function or
PromiseResolveThenableJob which aren't debug related. But changed anyway since
it's used only for debug purposes.

https://codereview.chromium.org/2415023002/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2415023002/diff/40001/src/isolate.cc#newcode3079
src/isolate.cc:3079: !name_->IsUndefined(isolate_)) {
I wonder if it'd be better to do a id_->IsNumber() && name_->IsString() check
here

[commit-bot: I haz the power, 2016-10-13 21:42:14]

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

[commit-bot: I haz the power, 2016-10-13 21:42:15]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[adamk, 2016-10-14 20:34:51]

lgtm

Yang, can you take a look for debug/?

https://codereview.chromium.org/2415023002/diff/40001/src/heap-symbols.h
File src/heap-symbols.h (right):

https://codereview.chromium.org/2415023002/diff/40001/src/heap-symbols.h#newc...
src/heap-symbols.h:175: V(will_handle_string, "willHandle")                     
        \
Nit: "i" goes after "e" :)

https://codereview.chromium.org/2415023002/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2415023002/diff/40001/src/isolate.cc#newcode3079
src/isolate.cc:3079: !name_->IsUndefined(isolate_)) {
On 2016/10/13 21:27:30, gsathya wrote:
> I wonder if it'd be better to do a id_->IsNumber() && name_->IsString() check
> here

Yeah, that seems better to me.

[gsathya, 2016-10-14 21:27:23]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 21:27:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 21:56:07]

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

[commit-bot: I haz the power, 2016-10-14 21:56:08]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-10-14 22:21:27]

https://codereview.chromium.org/2415023002/diff/40001/src/heap-symbols.h
File src/heap-symbols.h (right):

https://codereview.chromium.org/2415023002/diff/40001/src/heap-symbols.h#newc...
src/heap-symbols.h:175: V(will_handle_string, "willHandle")                     
        \
On 2016/10/14 20:34:51, adamk wrote:
> Nit: "i" goes after "e" :)

Done.

https://codereview.chromium.org/2415023002/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2415023002/diff/40001/src/isolate.cc#newcode3079
src/isolate.cc:3079: !name_->IsUndefined(isolate_)) {
On 2016/10/14 20:34:51, adamk wrote:
> On 2016/10/13 21:27:30, gsathya wrote:
> > I wonder if it'd be better to do a id_->IsNumber() && name_->IsString()
check
> > here
> 
> Yeah, that seems better to me.

Done.

[Yang, 2016-10-17 13:20:18]

On 2016/10/14 22:21:27, gsathya wrote:
> https://codereview.chromium.org/2415023002/diff/40001/src/heap-symbols.h
> File src/heap-symbols.h (right):
> 
>
https://codereview.chromium.org/2415023002/diff/40001/src/heap-symbols.h#newc...
> src/heap-symbols.h:175: V(will_handle_string, "willHandle")                   
 
>         \
> On 2016/10/14 20:34:51, adamk wrote:
> > Nit: "i" goes after "e" :)
> 
> Done.
> 
> https://codereview.chromium.org/2415023002/diff/40001/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://codereview.chromium.org/2415023002/diff/40001/src/isolate.cc#newcode3079
> src/isolate.cc:3079: !name_->IsUndefined(isolate_)) {
> On 2016/10/14 20:34:51, adamk wrote:
> > On 2016/10/13 21:27:30, gsathya wrote:
> > > I wonder if it'd be better to do a id_->IsNumber() && name_->IsString()
> check
> > > here
> > 
> > Yeah, that seems better to me.
> 
> Done.

lgtm.

[gsathya, 2016-10-17 15:10:15]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2016-10-17 15:10:16]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2415023002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-17 15:10:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 15:37:57]

Description was changed from

==========
[promises] Move async debug event creation to c++

BUG=v8:5343
==========

to

==========
[promises] Move async debug event creation to c++

BUG=v8:5343
==========

[commit-bot: I haz the power, 2016-10-17 15:37:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-17 15:38:25]

Description was changed from

==========
[promises] Move async debug event creation to c++

BUG=v8:5343
==========

to

==========
[promises] Move async debug event creation to c++

BUG=v8:5343

Committed: https://crrev.com/18a116c7bdf1bcd6a3d6b9419bbc98fffd28fd45
Cr-Commit-Position: refs/heads/master@{#40369}
==========

[commit-bot: I haz the power, 2016-10-17 15:38:26]

Patchset 5 (id:??) landed as
https://crrev.com/18a116c7bdf1bcd6a3d6b9419bbc98fffd28fd45
Cr-Commit-Position: refs/heads/master@{#40369}