Introduce more debug events for promises.

Introduce more debug events for promises.

BUG=v8:3093
LOG=Y
R=yangguo@chromium.org

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

[Alexandra Mikhaylova, 2014-07-17 16:31:09]

WIP

[aandrey, 2014-07-18 12:10:44]

https://codereview.chromium.org/393283007/diff/50001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/393283007/diff/50001/src/promise.js#newcode41
src/promise.js:41: var promise = PromiseInit(this);
this will fire "update" event before "new". please create a test.

also would be great not to generate "update" right after "new" event.

https://codereview.chromium.org/393283007/diff/50001/src/promise.js#newcode43
src/promise.js:43: %DebugPromiseEvent({ type : "new Promise",
"new"

https://codereview.chromium.org/393283007/diff/50001/src/promise.js#newcode66
src/promise.js:66: %DebugPromiseEvent({ type: "update Promise status",
"update"

https://codereview.chromium.org/393283007/diff/50001/src/promise.js#newcode244
src/promise.js:244: %DebugPromiseEvent({ type: "update Promise parent",
"chain"

https://codereview.chromium.org/393283007/diff/50001/test/mjsunit/es6/debug-p...
File test/mjsunit/es6/debug-promises-new-event.js (right):

https://codereview.chromium.org/393283007/diff/50001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-new-event.js:15: if (event_data instanceof
debug.NewPromiseEvent) {
if (!(event_data instanceof debug.NewPromiseEvent)) return;

[Alexandra Mikhaylova, 2014-07-22 10:45:36]

https://codereview.chromium.org/393283007/diff/50001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/393283007/diff/50001/src/promise.js#newcode41
src/promise.js:41: var promise = PromiseInit(this);
On 2014/07/18 12:10:44, aandrey wrote:
> this will fire "update" event before "new". please create a test.
> 
> also would be great not to generate "update" right after "new" event.

Changed the instrumentation: moved the "update" event out of PromiseSet.

https://codereview.chromium.org/393283007/diff/50001/src/promise.js#newcode43
src/promise.js:43: %DebugPromiseEvent({ type : "new Promise",
On 2014/07/18 12:10:44, aandrey wrote:
> "new"

Done.

https://codereview.chromium.org/393283007/diff/50001/src/promise.js#newcode66
src/promise.js:66: %DebugPromiseEvent({ type: "update Promise status",
On 2014/07/18 12:10:44, aandrey wrote:
> "update"

Done.

https://codereview.chromium.org/393283007/diff/50001/src/promise.js#newcode244
src/promise.js:244: %DebugPromiseEvent({ type: "update Promise parent",
On 2014/07/18 12:10:44, aandrey wrote:
> "chain"

Done.

https://codereview.chromium.org/393283007/diff/50001/test/mjsunit/es6/debug-p...
File test/mjsunit/es6/debug-promises-new-event.js (right):

https://codereview.chromium.org/393283007/diff/50001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-new-event.js:15: if (event_data instanceof
debug.NewPromiseEvent) {
On 2014/07/18 12:10:44, aandrey wrote:
> if (!(event_data instanceof debug.NewPromiseEvent)) return;

Done.

[aandrey, 2014-07-22 11:58:49]

https://codereview.chromium.org/393283007/diff/70001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/393283007/diff/70001/src/promise.js#newcode226
src/promise.js:226: %DebugPromiseEvent({ type: "update",
now 3 update event calls instead of one doesn't look good. just revert to the
previous version and send the event "if (DEBUG_IS_ACTIVE && status)".

https://codereview.chromium.org/393283007/diff/70001/test/mjsunit/es6/debug-p...
File test/mjsunit/es6/debug-promises-update-parent-event.js (right):

https://codereview.chromium.org/393283007/diff/70001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-update-parent-event.js:31:
assertSame(parentPromise, p2);
how many UpdatePromiseParentEvent happened by this time?
this is to test only the latest one?

[Alexandra Mikhaylova, 2014-07-22 12:47:22]

https://codereview.chromium.org/393283007/diff/70001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/393283007/diff/70001/src/promise.js#newcode226
src/promise.js:226: %DebugPromiseEvent({ type: "update",
On 2014/07/22 11:58:49, aandrey wrote:
> now 3 update event calls instead of one doesn't look good. just revert to the
> previous version and send the event "if (DEBUG_IS_ACTIVE && status)".

Done.

https://codereview.chromium.org/393283007/diff/70001/test/mjsunit/es6/debug-p...
File test/mjsunit/es6/debug-promises-update-parent-event.js (right):

https://codereview.chromium.org/393283007/diff/70001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-update-parent-event.js:31:
assertSame(parentPromise, p2);
On 2014/07/22 11:58:49, aandrey wrote:
> how many UpdatePromiseParentEvent happened by this time?
> this is to test only the latest one?

There should be 3 UpdatePromiseParentEvent's (added a check), and we check the
result of the latest one.

[aandrey, 2014-07-22 12:54:06]

looks good, deferring to @yangguo and @rossberg

[Yang, 2014-07-24 08:43:58]

LGTM with comments.

https://codereview.chromium.org/393283007/diff/90001/src/debug-debugger.js
File src/debug-debugger.js (right):

https://codereview.chromium.org/393283007/diff/90001/src/debug-debugger.js#ne...
src/debug-debugger.js:1211: event = new UpdatePromiseParentEvent(event_data);
unless you want to add some common logic later, I don't see why you can't just
return the event in each if-body directly.

https://codereview.chromium.org/393283007/diff/90001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/393283007/diff/90001/src/promise.js#newcode65
src/promise.js:65: if (DEBUG_IS_ACTIVE && status) {
I'd prefer a more explicit "status === 0" here, since the status constants are
sort of arbitrary. It would also mirror line 80.

https://codereview.chromium.org/393283007/diff/90001/test/mjsunit/es6/debug-p...
File test/mjsunit/es6/debug-promises-update-parent-event.js (right):

https://codereview.chromium.org/393283007/diff/90001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-update-parent-event.js:22: parentPromise =
event_data.parentPromise().value();
how about, instead of using a single value for parentPromise, log the parent
promises into an array, and compare the array with the expectation?

https://codereview.chromium.org/393283007/diff/90001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-update-parent-event.js:33: var p3 = p2.then();
Instead of a chain of promises, how about having a tree of promises? like adding
var p4 = p1.then();

[Alexandra Mikhaylova, 2014-07-24 09:18:37]

https://codereview.chromium.org/393283007/diff/90001/src/debug-debugger.js
File src/debug-debugger.js (right):

https://codereview.chromium.org/393283007/diff/90001/src/debug-debugger.js#ne...
src/debug-debugger.js:1211: event = new UpdatePromiseParentEvent(event_data);
On 2014/07/24 08:43:58, Yang wrote:
> unless you want to add some common logic later, I don't see why you can't just
> return the event in each if-body directly.

Fixed it.

https://codereview.chromium.org/393283007/diff/90001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/393283007/diff/90001/src/promise.js#newcode65
src/promise.js:65: if (DEBUG_IS_ACTIVE && status) {
On 2014/07/24 08:43:58, Yang wrote:
> I'd prefer a more explicit "status === 0" here, since the status constants are
> sort of arbitrary. It would also mirror line 80.

Done.

https://codereview.chromium.org/393283007/diff/90001/test/mjsunit/es6/debug-p...
File test/mjsunit/es6/debug-promises-update-parent-event.js (right):

https://codereview.chromium.org/393283007/diff/90001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-update-parent-event.js:22: parentPromise =
event_data.parentPromise().value();
On 2014/07/24 08:43:58, Yang wrote:
> how about, instead of using a single value for parentPromise, log the parent
> promises into an array, and compare the array with the expectation?

Thanks, done. Added an array of promise/parent pairs.

https://codereview.chromium.org/393283007/diff/90001/test/mjsunit/es6/debug-p...
test/mjsunit/es6/debug-promises-update-parent-event.js:33: var p3 = p2.then();
On 2014/07/24 08:43:58, Yang wrote:
> Instead of a chain of promises, how about having a tree of promises? like
adding
> var p4 = p1.then();

Done.

[Alexandra Mikhaylova, 2014-07-24 10:27:29]

On 2014/07/24 09:18:37, Alexandra Mikhaylova wrote:
> https://codereview.chromium.org/393283007/diff/90001/src/debug-debugger.js
> File src/debug-debugger.js (right):
> 
>
https://codereview.chromium.org/393283007/diff/90001/src/debug-debugger.js#ne...
> src/debug-debugger.js:1211: event = new UpdatePromiseParentEvent(event_data);
> On 2014/07/24 08:43:58, Yang wrote:
> > unless you want to add some common logic later, I don't see why you can't
just
> > return the event in each if-body directly.
> 
> Fixed it.
> 
> https://codereview.chromium.org/393283007/diff/90001/src/promise.js
> File src/promise.js (right):
> 
> https://codereview.chromium.org/393283007/diff/90001/src/promise.js#newcode65
> src/promise.js:65: if (DEBUG_IS_ACTIVE && status) {
> On 2014/07/24 08:43:58, Yang wrote:
> > I'd prefer a more explicit "status === 0" here, since the status constants
are
> > sort of arbitrary. It would also mirror line 80.
> 
> Done.
> 
>
https://codereview.chromium.org/393283007/diff/90001/test/mjsunit/es6/debug-p...
> File test/mjsunit/es6/debug-promises-update-parent-event.js (right):
> 
>
https://codereview.chromium.org/393283007/diff/90001/test/mjsunit/es6/debug-p...
> test/mjsunit/es6/debug-promises-update-parent-event.js:22: parentPromise =
> event_data.parentPromise().value();
> On 2014/07/24 08:43:58, Yang wrote:
> > how about, instead of using a single value for parentPromise, log the parent
> > promises into an array, and compare the array with the expectation?
> 
> Thanks, done. Added an array of promise/parent pairs.
> 
>
https://codereview.chromium.org/393283007/diff/90001/test/mjsunit/es6/debug-p...
> test/mjsunit/es6/debug-promises-update-parent-event.js:33: var p3 = p2.then();
> On 2014/07/24 08:43:58, Yang wrote:
> > Instead of a chain of promises, how about having a tree of promises? like
> adding
> > var p4 = p1.then();
> 
> Done.

Actually, we're thinking of merging these 3 "subtypes" of PromiseEvent
(NewPromiseEvent, UpdatePromiseStatusEvent & UpdatePromiseParentEvent) into one
with a "type" field.
This would be much more convenient to handle in the DevTools.

WDYT?

[Yang, 2014-07-24 10:30:03]

On 2014/07/24 10:27:29, Alexandra Mikhaylova wrote:
> On 2014/07/24 09:18:37, Alexandra Mikhaylova wrote:
> > https://codereview.chromium.org/393283007/diff/90001/src/debug-debugger.js
> > File src/debug-debugger.js (right):
> > 
> >
>
https://codereview.chromium.org/393283007/diff/90001/src/debug-debugger.js#ne...
> > src/debug-debugger.js:1211: event = new
UpdatePromiseParentEvent(event_data);
> > On 2014/07/24 08:43:58, Yang wrote:
> > > unless you want to add some common logic later, I don't see why you can't
> just
> > > return the event in each if-body directly.
> > 
> > Fixed it.
> > 
> > https://codereview.chromium.org/393283007/diff/90001/src/promise.js
> > File src/promise.js (right):
> > 
> >
https://codereview.chromium.org/393283007/diff/90001/src/promise.js#newcode65
> > src/promise.js:65: if (DEBUG_IS_ACTIVE && status) {
> > On 2014/07/24 08:43:58, Yang wrote:
> > > I'd prefer a more explicit "status === 0" here, since the status constants
> are
> > > sort of arbitrary. It would also mirror line 80.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/393283007/diff/90001/test/mjsunit/es6/debug-p...
> > File test/mjsunit/es6/debug-promises-update-parent-event.js (right):
> > 
> >
>
https://codereview.chromium.org/393283007/diff/90001/test/mjsunit/es6/debug-p...
> > test/mjsunit/es6/debug-promises-update-parent-event.js:22: parentPromise =
> > event_data.parentPromise().value();
> > On 2014/07/24 08:43:58, Yang wrote:
> > > how about, instead of using a single value for parentPromise, log the
parent
> > > promises into an array, and compare the array with the expectation?
> > 
> > Thanks, done. Added an array of promise/parent pairs.
> > 
> >
>
https://codereview.chromium.org/393283007/diff/90001/test/mjsunit/es6/debug-p...
> > test/mjsunit/es6/debug-promises-update-parent-event.js:33: var p3 =
p2.then();
> > On 2014/07/24 08:43:58, Yang wrote:
> > > Instead of a chain of promises, how about having a tree of promises? like
> > adding
> > > var p4 = p1.then();
> > 
> > Done.
> 
> Actually, we're thinking of merging these 3 "subtypes" of PromiseEvent
> (NewPromiseEvent, UpdatePromiseStatusEvent & UpdatePromiseParentEvent) into
one
> with a "type" field.
> This would be much more convenient to handle in the DevTools.
> 
> WDYT?

That's definitely possile. But Let's do that in a future CL.

[Yang, 2014-07-24 10:43:49]

Committed patchset #8 manually as r22591 (presubmit successful).