[promises] Move PromiseResolveThenableJob to c++

[promises] Move PromiseResolveThenableJob to c++

- Add a new container object to store the data required for
PromiseResolveThenableJob.

- Create a new runtime function to enqueue the microtask event with
the required data.

This patches causes a 4% regression in the bluebird benchmark.

BUG=v8:5343
Committed: https://crrev.com/8c87ae9b880f0b6c6824a68b39aec6a204d566c1
Cr-Commit-Position: refs/heads/master@{#39571}

[gsathya, 2016-09-06 20:53:40]

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

[gsathya, 2016-09-06 20:53:40]

I still need to fix the debug calls. Also I'm not too happy with the name
PromiseContainer, let me know if you can think of something better.

[adamk, 2016-09-06 21:04:57]

Some initial comments; I do expect the debug stuff to be the trickiest thing
here, and Dan's much more knowledgeable about that than me.

https://codereview.chromium.org/2314903004/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2314903004/diff/1/src/objects.h#newcode6582
src/objects.h:6582: class PromiseContainer : public Struct {
Please add a class comment explaining what this is for.

https://codereview.chromium.org/2314903004/diff/1/src/objects.h#newcode6584
src/objects.h:6584: DECL_ACCESSORS(promise, Object)
Do we not have any more specific type for promise?

https://codereview.chromium.org/2314903004/diff/1/src/runtime/runtime-interna...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2314903004/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:562: CONVERT_ARG_HANDLE_CHECKED(JSObject,
resolution, 1);
The struct says these are just Objects; which is it?

[gsathya, 2016-09-07 04:26:31]

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

[commit-bot: I haz the power, 2016-09-07 04:26: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-09-07 04:27:42]

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

[commit-bot: I haz the power, 2016-09-07 04:27:46]

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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12267)
  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/8354)

[gsathya, 2016-09-07 04:28:46]

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

[commit-bot: I haz the power, 2016-09-07 04:28:52]

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-09-07 04:30:02]

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

[commit-bot: I haz the power, 2016-09-07 04:30:03]

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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12268)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)

[gsathya, 2016-09-07 04:33:35]

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

[commit-bot: I haz the power, 2016-09-07 04:33: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-09-07 04:37:49]

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

[commit-bot: I haz the power, 2016-09-07 04:37:49]

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

[gsathya, 2016-09-07 04:45:30]

Fixed the debug calls. There's a fair bit of duplicate code in the debug{.cc,
.js} code, that I'm not too happy about. But this should be temporary until we
move the rest of the promise implementation over to c++.

https://codereview.chromium.org/2314903004/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2314903004/diff/1/src/objects.h#newcode6582
src/objects.h:6582: class PromiseContainer : public Struct {
On 2016/09/06 21:04:57, adamk wrote:
> Please add a class comment explaining what this is for.

Done.

https://codereview.chromium.org/2314903004/diff/1/src/objects.h#newcode6584
src/objects.h:6584: DECL_ACCESSORS(promise, Object)
On 2016/09/06 21:04:57, adamk wrote:
> Do we not have any more specific type for promise?

Nope, we only have an instance type of JS_PROMISE_TYPE which gets tacked on to
the promise constructor in the bootstrapper. But no concrete promise class.

https://codereview.chromium.org/2314903004/diff/1/src/runtime/runtime-interna...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2314903004/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:562: CONVERT_ARG_HANDLE_CHECKED(JSObject,
resolution, 1);
On 2016/09/06 21:04:57, adamk wrote:
> The struct says these are just Objects; which is it?

Updated the struct to be more specific.

[gsathya, 2016-09-08 07:13:46]

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

[commit-bot: I haz the power, 2016-09-08 07:14:00]

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

[gsathya, 2016-09-08 07:24:40]

Description was changed from

==========
[promises] Move PromiseResolveThenableJob to c++

- Add a new container object to store the data required for
PromiseResolveThenableJob.

- Create a new runtime function to enqueue the microtask event with
the required data.


BUG=v8:5046,v8:5343
==========

to

==========
[promises] Move PromiseResolveThenableJob to c++

- Add a new container object to store the data required for
PromiseResolveThenableJob.

- Create a new runtime function to enqueue the microtask event with
the required data.

This patches causes a 4% regression in the bluebird benchmark.

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

[gsathya, 2016-09-08 07:35:57]

PTAL. Fixed test. But regressed on the benchmark :(

[commit-bot: I haz the power, 2016-09-08 07:55:52]

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

[commit-bot: I haz the power, 2016-09-08 07:55:53]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-09-08 20:46:04]

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

[commit-bot: I haz the power, 2016-09-08 20:46:11]

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-09-08 20:51:21]

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

[commit-bot: I haz the power, 2016-09-08 20:51:22]

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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12411)
  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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/12342)

[gsathya, 2016-09-08 21:02:35]

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

[commit-bot: I haz the power, 2016-09-08 21:02:41]

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-09-08 21:15:32]

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

[commit-bot: I haz the power, 2016-09-08 21:15:32]

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_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[gsathya, 2016-09-08 21:21:07]

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

[commit-bot: I haz the power, 2016-09-08 21:21:12]

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-09-08 21:40:37]

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

[commit-bot: I haz the power, 2016-09-08 21:40:38]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/13931)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[gsathya, 2016-09-08 21:43:49]

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

[commit-bot: I haz the power, 2016-09-08 21:43:59]

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

[gsathya, 2016-09-08 22:05:41]

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

[commit-bot: I haz the power, 2016-09-08 22:05:47]

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

[adamk, 2016-09-08 22:28:18]

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2918: MaybeHandle<Object> maybe_exception;
This should be declared right before it's used.

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2925: Handle<Object>
after_debug_event(container->after_debug_event(), this);
I don't think you need all these handles up front, I'd create them as they're
necessary.

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2926: Handle<Object> argv[] = {resolve, reject};
This can also move down to right before the call

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2928: if (debug()->is_active() &&
(*before_debug_event)->IsJSObject()) {
Handles already overload operator-> so you don't need the double dereference.

Is this check needed in case debug is enabled between the time the job is
enqueued and when this code runs?

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2929: Handle<JSObject>
event_data(JSObject::cast(*before_debug_event), this);
No need to create a new handle here...

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2930: debug()->OnAsyncTaskEvent(event_data);
...instead use Handle<JSObject>::cast(before_debug_event)

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2944: if (debug()->is_active() &&
(*after_debug_event)->IsJSObject()) {
Same applies here and below to the handling of after_debug_event

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2997: SaveContext save(this);
As discussed I think you might need to use most of this context handling stuff
as well.

I see some blink tests failing with suggestive-sounding error messages that
mention "contexts".

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

https://codereview.chromium.org/2314903004/diff/210001/src/objects-debug.cc#n...
src/objects-debug.cc:909: VerifyPointer(reject());
Why aren't these also using ObjectVerify()?

[commit-bot: I haz the power, 2016-09-08 22:31:41]

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

[commit-bot: I haz the power, 2016-09-08 22:31:42]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-09-09 18:20:09]

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

[commit-bot: I haz the power, 2016-09-09 18:20:18]

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-09-09 18:59:56]

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

[commit-bot: I haz the power, 2016-09-09 18:59:57]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-09-13 23:05:15]

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

[commit-bot: I haz the power, 2016-09-13 23:05:17]

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-09-13 23:40:24]

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

[commit-bot: I haz the power, 2016-09-13 23:40:25]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-09-14 00:39:14]

Blink test failures seem unrelated. PTAL

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2918: MaybeHandle<Object> maybe_exception;
On 2016/09/08 22:28:18, adamk wrote:
> This should be declared right before it's used.

Done.

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2925: Handle<Object>
after_debug_event(container->after_debug_event(), this);
On 2016/09/08 22:28:18, adamk wrote:
> I don't think you need all these handles up front, I'd create them as they're
> necessary.

Done.

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2926: Handle<Object> argv[] = {resolve, reject};
On 2016/09/08 22:28:18, adamk wrote:
> This can also move down to right before the call

Done.

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2928: if (debug()->is_active() &&
(*before_debug_event)->IsJSObject()) {
On 2016/09/08 22:28:17, adamk wrote:
> Handles already overload operator-> so you don't need the double dereference.

Done.

> Is this check needed in case debug is enabled between the time the job is
> enqueued and when this code runs?

Yeah.

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2929: Handle<JSObject>
event_data(JSObject::cast(*before_debug_event), this);
On 2016/09/08 22:28:17, adamk wrote:
> No need to create a new handle here...

Done.

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2930: debug()->OnAsyncTaskEvent(event_data);
On 2016/09/08 22:28:17, adamk wrote:
> ...instead use Handle<JSObject>::cast(before_debug_event)

Done.

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2944: if (debug()->is_active() &&
(*after_debug_event)->IsJSObject()) {
On 2016/09/08 22:28:17, adamk wrote:
> Same applies here and below to the handling of after_debug_event

Done.

https://codereview.chromium.org/2314903004/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2997: SaveContext save(this);
On 2016/09/08 22:28:17, adamk wrote:
> As discussed I think you might need to use most of this context handling stuff
> as well.
> 
> I see some blink tests failing with suggestive-sounding error messages that
> mention "contexts".

Done.

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

https://codereview.chromium.org/2314903004/diff/210001/src/objects-debug.cc#n...
src/objects-debug.cc:909: VerifyPointer(reject());
On 2016/09/08 22:28:18, adamk wrote:
> Why aren't these also using ObjectVerify()?

The other structs use VerifyPointer so I made all of PromiseContainer use that
initially, but when I refactored it I changed it to ObjectVerify because it
seems to be a stronger check. Updated these to use ObjectVerify too.

[adamk, 2016-09-14 22:52:51]

Why do you think the blink tests are unrelated?

https://codereview.chromium.org/2314903004/diff/270001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2314903004/diff/270001/src/isolate.cc#newcode...
src/isolate.cc:2930: Handle<JSObject> promise(container->promise(), this);
This doesn't seem to be used at all. Why is it passed in?

https://codereview.chromium.org/2314903004/diff/270001/src/isolate.cc#newcode...
src/isolate.cc:2942: Execution::Call(this, reject, factory()->undefined_value(),
What happens if the reject handler throws?

https://codereview.chromium.org/2314903004/diff/270001/src/runtime/runtime-in...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2314903004/diff/270001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:561: CONVERT_ARG_HANDLE_CHECKED(JSObject,
promise, 0);
It looks to me like this argument is unused (see comment in isolate.cc).

https://codereview.chromium.org/2314903004/diff/270001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:562: CONVERT_ARG_HANDLE_CHECKED(JSObject,
resolution, 1);
Couldn't resolution be a Proxy? I see the call to this runtime function guarded
by IS_RECEIVER(), but I think that includes Proxies. If that's so, this would
need to be a JSReceiver instead of a JSObject. Can you check if there's a test
that ends up passing a Proxy here? It looks like

Promise.resolve(someThenableProxy)

should do the trick.

[gsathya, 2016-09-19 19:33:04]

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

[commit-bot: I haz the power, 2016-09-19 19:33:07]

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-09-19 20:42:19]

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

[commit-bot: I haz the power, 2016-09-19 20:42:21]

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

[gsathya, 2016-09-19 20:46:15]

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

[commit-bot: I haz the power, 2016-09-19 20:46:19]

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

[gsathya, 2016-09-19 20:49:12]

Thanks for the review!

On 2016/09/14 22:52:51, adamk wrote:
> Why do you think the blink tests are unrelated?

They were already broken, and they seem to pass now. (trigged the bots on the
latest patch now).

> https://codereview.chromium.org/2314903004/diff/270001/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://codereview.chromium.org/2314903004/diff/270001/src/isolate.cc#newcode...
> src/isolate.cc:2930: Handle<JSObject> promise(container->promise(), this);
> This doesn't seem to be used at all. Why is it passed in?

Removed.
 
>
https://codereview.chromium.org/2314903004/diff/270001/src/isolate.cc#newcode...
> src/isolate.cc:2942: Execution::Call(this, reject,
factory()->undefined_value(),
> What happens if the reject handler throws?

Updated to use TryCall. Happy to add a comment about ignoring the exception if
you think it's worth it.
 
>
https://codereview.chromium.org/2314903004/diff/270001/src/runtime/runtime-in...
> File src/runtime/runtime-internal.cc (right):
> 
>
https://codereview.chromium.org/2314903004/diff/270001/src/runtime/runtime-in...
> src/runtime/runtime-internal.cc:561: CONVERT_ARG_HANDLE_CHECKED(JSObject,
> promise, 0);
> It looks to me like this argument is unused (see comment in isolate.cc).

Removed.

>
https://codereview.chromium.org/2314903004/diff/270001/src/runtime/runtime-in...
> src/runtime/runtime-internal.cc:562: CONVERT_ARG_HANDLE_CHECKED(JSObject,
> resolution, 1);
> Couldn't resolution be a Proxy? I see the call to this runtime function
guarded
> by IS_RECEIVER(), but I think that includes Proxies. If that's so, this would
> need to be a JSReceiver instead of a JSObject. Can you check if there's a test
> that ends up passing a Proxy here? It looks like
> 
> Promise.resolve(someThenableProxy)
> 
> should do the trick.

Yeah, you're right. There wasn't a test for this. I added one and changed
resolution to be a JSReceiver.

[commit-bot: I haz the power, 2016-09-19 21:31:58]

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

[commit-bot: I haz the power, 2016-09-19 21:31:58]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-09-19 21:42:35]

Almost there...

https://codereview.chromium.org/2314903004/diff/290001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2314903004/diff/290001/src/isolate.cc#newcode...
src/isolate.cc:2985: MaybeHandle<Object> status =
No need to assign this anymore, now that TryCall swallows the exception.

https://codereview.chromium.org/2314903004/diff/290001/src/isolate.cc#newcode...
src/isolate.cc:3055: if (result.is_null() && maybe_exception.is_null()) {
It worries me a little bit that we're now missing this code. Could you factor it
out of the if-else somehow?

For a little background: it's trying to determine if the exception that was
thrown is due to someone calling Isolate::TerminateExecution, e.g. if this is a
Worker thread being torn down.

[gsathya, 2016-09-20 01:23:29]

On 2016/09/19 21:42:35, adamk wrote:
> Almost there...
> 
> https://codereview.chromium.org/2314903004/diff/290001/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://codereview.chromium.org/2314903004/diff/290001/src/isolate.cc#newcode...
> src/isolate.cc:2985: MaybeHandle<Object> status =
> No need to assign this anymore, now that TryCall swallows the exception.

Done.

>
https://codereview.chromium.org/2314903004/diff/290001/src/isolate.cc#newcode...
> src/isolate.cc:3055: if (result.is_null() && maybe_exception.is_null()) {
> It worries me a little bit that we're now missing this code. Could you factor
it
> out of the if-else somehow?
> 
> For a little background: it's trying to determine if the exception that was
> thrown is due to someone calling Isolate::TerminateExecution, e.g. if this is
a
> Worker thread being torn down.

Done.

[gsathya, 2016-09-20 01:23:45]

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

[commit-bot: I haz the power, 2016-09-20 01:23:50]

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-09-20 02:16:09]

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

[commit-bot: I haz the power, 2016-09-20 02:16:10]

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

[gsathya, 2016-09-20 16:55:45]

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

[commit-bot: I haz the power, 2016-09-20 16:55: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-09-20 17:16:26]

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

[commit-bot: I haz the power, 2016-09-20 17:16:32]

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...)

[gsathya, 2016-09-20 17:48:33]

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

[commit-bot: I haz the power, 2016-09-20 17:48:37]

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-09-20 18:50:32]

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

[commit-bot: I haz the power, 2016-09-20 18:50:34]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-09-20 20:08:29]

Like the new layout, some more comments on the details

https://codereview.chromium.org/2314903004/diff/350001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2314903004/diff/350001/src/isolate.cc#newcode...
src/isolate.cc:2983: Execution::TryCall(this, reject,
factory()->undefined_value(),
Should this be assigned to *result?

https://codereview.chromium.org/2314903004/diff/350001/src/isolate.cc#newcode...
src/isolate.cc:2984: arraysize(reason_arg), reason_arg);
I think you want to pass maybe_reason here too

https://codereview.chromium.org/2314903004/diff/350001/src/isolate.cc#newcode...
src/isolate.cc:3068: if (microtask->IsPromiseContainer()) {
Why if/if? shouldn't this be if/else?

[gsathya, 2016-09-20 22:42:48]

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

[commit-bot: I haz the power, 2016-09-20 22:42:57]

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-09-20 22:44:27]

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

[commit-bot: I haz the power, 2016-09-20 22:44:28]

Dry run: Try jobs failed on following builders:
  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_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_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/13125)
  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_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)

[gsathya, 2016-09-20 22:55:04]

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

[commit-bot: I haz the power, 2016-09-20 22:55:17]

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

[gsathya, 2016-09-20 23:09:50]

On 2016/09/20 20:08:29, adamk wrote:
> Like the new layout, some more comments on the details
> 
> https://codereview.chromium.org/2314903004/diff/350001/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://codereview.chromium.org/2314903004/diff/350001/src/isolate.cc#newcode...
> src/isolate.cc:2983: Execution::TryCall(this, reject,
> factory()->undefined_value(),
> Should this be assigned to *result?
>

Done.
 
>
https://codereview.chromium.org/2314903004/diff/350001/src/isolate.cc#newcode...
> src/isolate.cc:2984: arraysize(reason_arg), reason_arg);
> I think you want to pass maybe_reason here too

Done.

>
https://codereview.chromium.org/2314903004/diff/350001/src/isolate.cc#newcode...
> src/isolate.cc:3068: if (microtask->IsPromiseContainer()) {
> Why if/if? shouldn't this be if/else?

Done.

[adamk, 2016-09-20 23:24:58]

lgtm!

[commit-bot: I haz the power, 2016-09-20 23:32:34]

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

[commit-bot: I haz the power, 2016-09-20 23:32:35]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-09-20 23:33:17]

The CQ bit was checked by gsathya@chromium.org

[commit-bot: I haz the power, 2016-09-20 23:33:21]

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-09-20 23:37:50]

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

[commit-bot: I haz the power, 2016-09-20 23:37:52]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24558)

[gsathya, 2016-09-20 23:47:53]

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

[gsathya, 2016-09-20 23:48:48]

+Benedikt for compiler/

[Benedikt Meurer, 2016-09-21 03:46:29]

LGTM on compiler.

[gsathya, 2016-09-21 03:46:49]

The CQ bit was checked by gsathya@chromium.org

[commit-bot: I haz the power, 2016-09-21 03:46:54]

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-09-21 03:49:36]

Description was changed from

==========
[promises] Move PromiseResolveThenableJob to c++

- Add a new container object to store the data required for
PromiseResolveThenableJob.

- Create a new runtime function to enqueue the microtask event with
the required data.

This patches causes a 4% regression in the bluebird benchmark.

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

to

==========
[promises] Move PromiseResolveThenableJob to c++

- Add a new container object to store the data required for
PromiseResolveThenableJob.

- Create a new runtime function to enqueue the microtask event with
the required data.

This patches causes a 4% regression in the bluebird benchmark.

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

[commit-bot: I haz the power, 2016-09-21 03:49:39]

Committed patchset #22 (id:410001)

[commit-bot: I haz the power, 2016-09-21 03:49:55]

Description was changed from

==========
[promises] Move PromiseResolveThenableJob to c++

- Add a new container object to store the data required for
PromiseResolveThenableJob.

- Create a new runtime function to enqueue the microtask event with
the required data.

This patches causes a 4% regression in the bluebird benchmark.

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

to

==========
[promises] Move PromiseResolveThenableJob to c++

- Add a new container object to store the data required for
PromiseResolveThenableJob.

- Create a new runtime function to enqueue the microtask event with
the required data.

This patches causes a 4% regression in the bluebird benchmark.

BUG=v8:5343

Committed: https://crrev.com/8c87ae9b880f0b6c6824a68b39aec6a204d566c1
Cr-Commit-Position: refs/heads/master@{#39571}
==========

[commit-bot: I haz the power, 2016-09-21 03:49:57]

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