[promisehook] Store promise in PromiseReactionJob

[promisehook] Store promise in PromiseReactionJob

This will be used in PromiseHook.

BUG=v8:4643
Review-Url: https://codereview.chromium.org/2581503003
Cr-Commit-Position: refs/heads/master@{#41730}
Committed: https://chromium.googlesource.com/v8/v8/+/b4aadaec1e46a98f2c2ee273c99aa808e19c770f

[gsathya, 2016-12-15 01:02:31]

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

[commit-bot: I haz the power, 2016-12-15 01:02:33]

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

[gsathya, 2016-12-15 01:07:11]

Description was changed from

==========
[promisehook] Store promise in PromiseReactionJob

BUG=v8:4643
==========

to

==========
[promisehook] Store promise in PromiseReactionJob

This will be used in PromiseHook.

BUG=v8:4643
==========

[commit-bot: I haz the power, 2016-12-15 01:08:22]

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

[commit-bot: I haz the power, 2016-12-15 01:08:23]

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

[gsathya, 2016-12-15 01:12:48]

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

[commit-bot: I haz the power, 2016-12-15 01:12:53]

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-12-15 01:39:00]

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

[commit-bot: I haz the power, 2016-12-15 01:39:02]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-15 01:53:01]

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

[adamk, 2016-12-15 07:00:37]

lgtm % comment below

(haven't actually looked at the PromiseHook CL yet, but assuming this is
required it seems OK)

https://codereview.chromium.org/2581503003/diff/30001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2581503003/diff/30001/src/objects.h#newcode6901
src/objects.h:6901: DECL_ACCESSORS(promise, JSPromise)
Please update objects-debug.cc while you're adding this.

[gsathya, 2016-12-15 14:37:27]

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

[commit-bot: I haz the power, 2016-12-15 14:37:33]

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

[gsathya, 2016-12-15 14:38:44]

On 2016/12/15 07:00:37, adamk wrote:
> lgtm % comment below
> 
> (haven't actually looked at the PromiseHook CL yet, but assuming this is
> required it seems OK)
> 
> https://codereview.chromium.org/2581503003/diff/30001/src/objects.h
> File src/objects.h (right):
> 
>
https://codereview.chromium.org/2581503003/diff/30001/src/objects.h#newcode6901
> src/objects.h:6901: DECL_ACCESSORS(promise, JSPromise)
> Please update objects-debug.cc while you're adding this.

Done. Updated objects-printer.cc also

[gsathya, 2016-12-15 15:11:49]

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

[commit-bot: I haz the power, 2016-12-15 15:11:59]

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-12-15 15:13:51]

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

[commit-bot: I haz the power, 2016-12-15 15:13:52]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17870)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/17939)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/17844)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30688)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/19465)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/19579)

[gsathya, 2016-12-15 15:20:09]

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

[commit-bot: I haz the power, 2016-12-15 15:20:22]

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-12-15 15:23:09]

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

[commit-bot: I haz the power, 2016-12-15 15:23:10]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/14271)

[gsathya, 2016-12-15 15:23:19]

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

[commit-bot: I haz the power, 2016-12-15 15:23:30]

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-12-15 15:48:21]

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

[commit-bot: I haz the power, 2016-12-15 15:48:22]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-15 15:48:48]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2016-12-15 15:48:48]

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/2581503003/#ps110001
(title: "rebase correctly")

[commit-bot: I haz the power, 2016-12-15 15:48:53]

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-12-15 15:50:55]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1481816928458750,
"parent_rev": "80b8a3c1a5a4c868f8a214f66372dc7d78b3c7c9", "commit_rev":
"b4aadaec1e46a98f2c2ee273c99aa808e19c770f"}

[commit-bot: I haz the power, 2016-12-15 15:50:59]

Description was changed from

==========
[promisehook] Store promise in PromiseReactionJob

This will be used in PromiseHook.

BUG=v8:4643
==========

to

==========
[promisehook] Store promise in PromiseReactionJob

This will be used in PromiseHook.

BUG=v8:4643

Review-Url: https://codereview.chromium.org/2581503003
Cr-Commit-Position: refs/heads/master@{#41730}
Committed:
https://chromium.googlesource.com/v8/v8/+/b4aadaec1e46a98f2c2ee273c99aa808e19...
==========

[commit-bot: I haz the power, 2016-12-15 15:51:00]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/v8/v8/+/b4aadaec1e46a98f2c2ee273c99aa808e19...

[kozy, 2016-12-15 16:07:58]

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

[kozy, 2016-12-15 16:07:59]

https://codereview.chromium.org/2581503003/diff/110001/src/builtins/builtins-...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2581503003/diff/110001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:503:
a->CallRuntime(Runtime::kEnqueuePromiseReactionJob, context, promise,
Does it mean that before promise can be collected when microtask is already
scheduled but not executed yet and now it's not a true?

var resolve;
var p = new Promise(resolveCb => resolve = resolveCb);
p.then(foo);
p = null;
resolve(); <- promise can be collected after this function call.

function foo() {
} <- now, promise will be collected after this function finished.

[gsathya, 2016-12-15 16:10:43]

On 2016/12/15 16:07:59, kozyatinskiy wrote:
>
https://codereview.chromium.org/2581503003/diff/110001/src/builtins/builtins-...
> File src/builtins/builtins-promise.cc (right):
> 
>
https://codereview.chromium.org/2581503003/diff/110001/src/builtins/builtins-...
> src/builtins/builtins-promise.cc:503:
> a->CallRuntime(Runtime::kEnqueuePromiseReactionJob, context, promise,
> Does it mean that before promise can be collected when microtask is already
> scheduled but not executed yet and now it's not a true?
> 
> var resolve;
> var p = new Promise(resolveCb => resolve = resolveCb);
> p.then(foo);
> p = null;
> resolve(); <- promise can be collected after this function call.
> 
> function foo() {
> } <- now, promise will be collected after this function finished.

Yes, you're correct.

[adamk, 2016-12-15 17:15:17]

On 2016/12/15 16:10:43, gsathya wrote:
> On 2016/12/15 16:07:59, kozyatinskiy wrote:
> >
>
https://codereview.chromium.org/2581503003/diff/110001/src/builtins/builtins-...
> > File src/builtins/builtins-promise.cc (right):
> > 
> >
>
https://codereview.chromium.org/2581503003/diff/110001/src/builtins/builtins-...
> > src/builtins/builtins-promise.cc:503:
> > a->CallRuntime(Runtime::kEnqueuePromiseReactionJob, context, promise,
> > Does it mean that before promise can be collected when microtask is already
> > scheduled but not executed yet and now it's not a true?
> > 
> > var resolve;
> > var p = new Promise(resolveCb => resolve = resolveCb);
> > p.then(foo);
> > p = null;
> > resolve(); <- promise can be collected after this function call.
> > 
> > function foo() {
> > } <- now, promise will be collected after this function finished.
> 
> Yes, you're correct.

kozyatinskiy, Is there a reason this causes concern?

[kozy, 2016-12-15 18:03:08]

On 2016/12/15 17:15:17, adamk wrote:
> On 2016/12/15 16:10:43, gsathya wrote:
> > On 2016/12/15 16:07:59, kozyatinskiy wrote:
> > >
> >
>
https://codereview.chromium.org/2581503003/diff/110001/src/builtins/builtins-...
> > > File src/builtins/builtins-promise.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2581503003/diff/110001/src/builtins/builtins-...
> > > src/builtins/builtins-promise.cc:503:
> > > a->CallRuntime(Runtime::kEnqueuePromiseReactionJob, context, promise,
> > > Does it mean that before promise can be collected when microtask is
already
> > > scheduled but not executed yet and now it's not a true?
> > > 
> > > var resolve;
> > > var p = new Promise(resolveCb => resolve = resolveCb);
> > > p.then(foo);
> > > p = null;
> > > resolve(); <- promise can be collected after this function call.
> > > 
> > > function foo() {
> > > } <- now, promise will be collected after this function finished.
> > 
> > Yes, you're correct.
> 
> kozyatinskiy, Is there a reason this causes concern?

I'm working on better async task instrumentation for inspector and with holding
promise I can simplify some logic, will upload CL soon.
And I'm just worried about two use cases:
1. for (var i = 0; i < N; ++i) Promise.resolve().then(foo) - we'll hold N
additional promises.
2. and it can hard to detect this additional memory in code that aggressively
use thenable jobs:
function fib(i) {
  if (i == 0) return Promise.resolve(1);
  if (i == 1) return Promise.resolve(1);
  return Promise.all([fib(i-1), fib(i-2)]).then((r) => r[0] + r[1]);
}

[adamk, 2016-12-15 18:08:06]

On 2016/12/15 18:03:08, kozyatinskiy wrote:
> On 2016/12/15 17:15:17, adamk wrote:
> > On 2016/12/15 16:10:43, gsathya wrote:
> > > On 2016/12/15 16:07:59, kozyatinskiy wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2581503003/diff/110001/src/builtins/builtins-...
> > > > File src/builtins/builtins-promise.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2581503003/diff/110001/src/builtins/builtins-...
> > > > src/builtins/builtins-promise.cc:503:
> > > > a->CallRuntime(Runtime::kEnqueuePromiseReactionJob, context, promise,
> > > > Does it mean that before promise can be collected when microtask is
> already
> > > > scheduled but not executed yet and now it's not a true?
> > > > 
> > > > var resolve;
> > > > var p = new Promise(resolveCb => resolve = resolveCb);
> > > > p.then(foo);
> > > > p = null;
> > > > resolve(); <- promise can be collected after this function call.
> > > > 
> > > > function foo() {
> > > > } <- now, promise will be collected after this function finished.
> > > 
> > > Yes, you're correct.
> > 
> > kozyatinskiy, Is there a reason this causes concern?
> 
> I'm working on better async task instrumentation for inspector and with
holding
> promise I can simplify some logic, will upload CL soon.
> And I'm just worried about two use cases:
> 1. for (var i = 0; i < N; ++i) Promise.resolve().then(foo) - we'll hold N
> additional promises.
> 2. and it can hard to detect this additional memory in code that aggressively
> use thenable jobs:
> function fib(i) {
>   if (i == 0) return Promise.resolve(1);
>   if (i == 1) return Promise.resolve(1);
>   return Promise.all([fib(i-1), fib(i-2)]).then((r) => r[0] + r[1]);
> }

Hmm. Would it make sense to only store the promise if promise hooks are actually
enabled (or, if debugging is enabled, for your async task instrumentation)?

[kozy, 2016-12-15 18:18:35]

On 2016/12/15 18:08:06, adamk wrote:
> On 2016/12/15 18:03:08, kozyatinskiy wrote:
> > I'm working on better async task instrumentation for inspector and with
> holding
> > promise I can simplify some logic, will upload CL soon.
> > And I'm just worried about two use cases:
> > 1. for (var i = 0; i < N; ++i) Promise.resolve().then(foo) - we'll hold N
> > additional promises.
> > 2. and it can hard to detect this additional memory in code that
aggressively
> > use thenable jobs:
> > function fib(i) {
> >   if (i == 0) return Promise.resolve(1);
> >   if (i == 1) return Promise.resolve(1);
> >   return Promise.all([fib(i-1), fib(i-2)]).then((r) => r[0] + r[1]);
> > }
> 
> Hmm. Would it make sense to only store the promise if promise hooks are
actually
> enabled (or, if debugging is enabled, for your async task instrumentation)?

In my change I'd like to send enqueueRecurring for all promises when promise is
resolved, cancel when promise is collected and all related microtasks are
finished. (more details on last two pages of
https://docs.google.com/a/google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJ...).
I need to have some logic in debug.cc code to track current amount of scheduled
task to send cancel not just when promise is collected but when it's collected
and all related mcirotasks are finished. (20 lines of code)
So I can implement my change without storing it. But if we're holding promise -
it'll allow me to simplify reporting and just report cancel when promise is
collected.

[kozy, 2016-12-15 18:19:55]

On 2016/12/15 18:08:06, adamk wrote:
> 
> Hmm. Would it make sense to only store the promise if promise hooks are
actually
> enabled (or, if debugging is enabled, for your async task instrumentation)?

And yes, on inspector side we don't need to store promises when async call
stacks are disabled (instrumentation is disabled).

[gsathya, 2016-12-15 18:28:47]

On 2016/12/15 18:03:08, kozyatinskiy wrote:
> On 2016/12/15 17:15:17, adamk wrote:
> > On 2016/12/15 16:10:43, gsathya wrote:
> > > On 2016/12/15 16:07:59, kozyatinskiy wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2581503003/diff/110001/src/builtins/builtins-...
> > > > File src/builtins/builtins-promise.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2581503003/diff/110001/src/builtins/builtins-...
> > > > src/builtins/builtins-promise.cc:503:
> > > > a->CallRuntime(Runtime::kEnqueuePromiseReactionJob, context, promise,
> > > > Does it mean that before promise can be collected when microtask is
> already
> > > > scheduled but not executed yet and now it's not a true?
> > > > 
> > > > var resolve;
> > > > var p = new Promise(resolveCb => resolve = resolveCb);
> > > > p.then(foo);
> > > > p = null;
> > > > resolve(); <- promise can be collected after this function call.
> > > > 
> > > > function foo() {
> > > > } <- now, promise will be collected after this function finished.
> > > 
> > > Yes, you're correct.
> > 
> > kozyatinskiy, Is there a reason this causes concern?
> 
> I'm working on better async task instrumentation for inspector and with
holding
> promise I can simplify some logic, will upload CL soon.
> And I'm just worried about two use cases:
> 1. for (var i = 0; i < N; ++i) Promise.resolve().then(foo) - we'll hold N
> additional promises.
> 2. and it can hard to detect this additional memory in code that aggressively
> use thenable jobs:
> function fib(i) {
>   if (i == 0) return Promise.resolve(1);
>   if (i == 1) return Promise.resolve(1);
>   return Promise.all([fib(i-1), fib(i-2)]).then((r) => r[0] + r[1]);
> }

The first example is valid, but I don't know if waiting until the next microtask
is a regression.

In the second example, we would hold all the promises anyway because the parent
promise isn't resolved until each child one is and they all hold references.
Similar to https://bugs.chromium.org/p/v8/issues/detail?id=5002