[promises] Move PromiseReactionJob to c++

[promises] Move PromiseReactionJob to c++

This patch results in a 11% improvement over 5 runs in the
bluebird benchmark.

BUG=v8:5343, v8:5046
TBR=bmeurer@chromium.org

Committed: https://crrev.com/6f94a8f1c7f0a94c74c5055b02b660d8e93fe5fe
Cr-Commit-Position: refs/heads/master@{#40239}

[gsathya, 2016-10-11 16:28:56]

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

[commit-bot: I haz the power, 2016-10-11 16:29:02]

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-11 16:32:31]

Description was changed from

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

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

to

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

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

[gsathya, 2016-10-11 16:32:31]

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

[commit-bot: I haz the power, 2016-10-11 16:33:49]

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

[commit-bot: I haz the power, 2016-10-11 16:33:55]

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-10-11 16:37:01]

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

[gsathya, 2016-10-11 16:37:25]

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

[commit-bot: I haz the power, 2016-10-11 16:37:30]

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-11 16:40:51]

https://codereview.chromium.org/2406343002/diff/20001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2406343002/diff/20001/src/js/promise.js#newco...
src/js/promise.js:220: function() {});
This is such a hack

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

https://codereview.chromium.org/2406343002/diff/20001/src/objects.h#newcode6715
src/objects.h:6715: class PromiseHandleInfo : public Struct {
PromiseReactionInfo is a better name?

[commit-bot: I haz the power, 2016-10-11 17:27:57]

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

[commit-bot: I haz the power, 2016-10-11 17:27:59]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-10-11 17:56:40]

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

[adamk, 2016-10-11 17:56:41]

https://codereview.chromium.org/2406343002/diff/20001/src/runtime/runtime-int...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2406343002/diff/20001/src/runtime/runtime-int...
src/runtime/runtime-internal.cc:586: reaction_context);
Rather than creating a function, I think you should be able to capture the
current native context here, from isolate->native_context().

[gsathya, 2016-10-11 18:14:53]

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

[commit-bot: I haz the power, 2016-10-11 18:15:04]

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-11 18:35:29]

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

[commit-bot: I haz the power, 2016-10-11 18:35:31]

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-10-11 22:41:57]

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

[commit-bot: I haz the power, 2016-10-11 22:42:02]

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-11 22:47:52]

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

[commit-bot: I haz the power, 2016-10-11 22:48: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-10-11 22:52:46]

On 2016/10/11 17:56:41, adamk wrote:
>
https://codereview.chromium.org/2406343002/diff/20001/src/runtime/runtime-int...
> File src/runtime/runtime-internal.cc (right):
> 
>
https://codereview.chromium.org/2406343002/diff/20001/src/runtime/runtime-int...
> src/runtime/runtime-internal.cc:586: reaction_context);
> Rather than creating a function, I think you should be able to capture the
> current native context here, from isolate->native_context().

PTAL

[gsathya, 2016-10-11 22:58:05]

Description was changed from

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

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

to

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

This patch results in a 12% improvement over 5 runs in the 
bluebird benchmark.

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

[adamk, 2016-10-11 23:14:30]

Sorry for all the naming nits below...

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3015
src/isolate.cc:3015: if (debug()->is_active()) {
I think you could create a helper class that'd encapsulate this work. You could
put it on the stack, passing |this|, before_debug_event, and after_debug_event,
and it would take care of firing the before_debug_event at construction and the
after_debug_event on destruction.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3026
src/isolate.cc:3026: if (tasks->IsJSArray()) {
Can you add a comment here about the two possible states tasks might be in?

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3029
src/isolate.cc:3029: Handle<FixedArrayBase> elements(array->elements(), this);
This is now unused.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3032
src/isolate.cc:3032: for (int i = 0; i < length; i += 2) {
Please add a DCHECK that length is even (otherwise this loop is unsafe)

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3033
src/isolate.cc:3033: Handle<Object> argv[] = {value, accessor->Get(array, i),
Please add the DCHECK for Has(i) and Has(i+1).

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3035
src/isolate.cc:3035: *result = Execution::TryCall(this, promise_handle(),
I'd hoist this out of the loop, you could store it in a handle the same place
where you create |value| and |tasks|.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3036
src/isolate.cc:3036: factory()->undefined_value(),
Same as the above, I'd hoist this out.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3037
src/isolate.cc:3037: arraysize(argv), argv, maybe_exception);
I'm not sure the same logic about result and maybe_exception holds as it did
before. This is only going to remember the last call.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3092
src/isolate.cc:3092: // TODO(gsathya): remove IsJSFunction?
You'd have to change the V8 API before you could do that, so I don't think this
TODO belongs here.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3171
src/isolate.cc:3171: if (microtask->IsPromiseContainer()) {
This should be merged with the else above

https://codereview.chromium.org/2406343002/diff/80001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2406343002/diff/80001/src/js/promise.js#newco...
src/js/promise.js:193: var id, name, before_debug, after_debug, instrumenting =
DEBUG_IS_ACTIVE;
Nit: s/before_debug/beforeDebug/

Same for after_debug

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

https://codereview.chromium.org/2406343002/diff/80001/src/objects-debug.cc#ne...
src/objects-debug.cc:932: reaction_context()->ObjectVerify();
I think I led you down the wrong path here the first time (still waiting for a
response on v8-dev to be sure). But I think the only checks you want here are
that fields are of the right type, e.g.,

CHECK(tasks()->IsJSArray() || tasks()->IsCallable())

https://codereview.chromium.org/2406343002/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2406343002/diff/80001/src/objects.h#newcode6687
src/objects.h:6687: class PromiseContainer : public Struct {
This name now sounds too generic. Maybe add a TODO to rename it?

https://codereview.chromium.org/2406343002/diff/80001/src/objects.h#newcode6714
src/objects.h:6714: class PromiseHandleInfo : public Struct {
This name is a little funny; why not PromiseReactionJobInfo?

https://codereview.chromium.org/2406343002/diff/80001/src/objects.h#newcode6718
src/objects.h:6718: DECL_ACCESSORS(deferred, Object)
Isn't this guaranteed to be a JSObject?

https://codereview.chromium.org/2406343002/diff/80001/src/objects.h#newcode6721
src/objects.h:6721: DECL_ACCESSORS(reaction_context, Context)
I think "context" would be a fine name for this.

[commit-bot: I haz the power, 2016-10-11 23:20:29]

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

[commit-bot: I haz the power, 2016-10-11 23:20:30]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-10-12 01:44:45]

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

[commit-bot: I haz the power, 2016-10-12 01:44:50]

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-12 01:47:28]

Description was changed from

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

This patch results in a 12% improvement over 5 runs in the 
bluebird benchmark.

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

to

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

This patch results in a 11% improvement over 5 runs in the 
bluebird benchmark.

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

[commit-bot: I haz the power, 2016-10-12 01:48:39]

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

[commit-bot: I haz the power, 2016-10-12 01:48:40]

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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/10582)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)

[gsathya, 2016-10-12 01:58:24]

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

[commit-bot: I haz the power, 2016-10-12 01:58:30]

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-12 02:00:44]

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3015
src/isolate.cc:3015: if (debug()->is_active()) {
On 2016/10/11 23:14:30, adamk wrote:
> I think you could create a helper class that'd encapsulate this work. You
could
> put it on the stack, passing |this|, before_debug_event, and
after_debug_event,
> and it would take care of firing the before_debug_event at construction and
the
> after_debug_event on destruction.

Done. But it's the same LoC

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3026
src/isolate.cc:3026: if (tasks->IsJSArray()) {
On 2016/10/11 23:14:30, adamk wrote:
> Can you add a comment here about the two possible states tasks might be in?

Done.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3029
src/isolate.cc:3029: Handle<FixedArrayBase> elements(array->elements(), this);
On 2016/10/11 23:14:30, adamk wrote:
> This is now unused.

Done.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3032
src/isolate.cc:3032: for (int i = 0; i < length; i += 2) {
On 2016/10/11 23:14:30, adamk wrote:
> Please add a DCHECK that length is even (otherwise this loop is unsafe)

Done.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3033
src/isolate.cc:3033: Handle<Object> argv[] = {value, accessor->Get(array, i),
On 2016/10/11 23:14:30, adamk wrote:
> Please add the DCHECK for Has(i) and Has(i+1).

Done.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3035
src/isolate.cc:3035: *result = Execution::TryCall(this, promise_handle(),
On 2016/10/11 23:14:30, adamk wrote:
> I'd hoist this out of the loop, you could store it in a handle the same place
> where you create |value| and |tasks|.

Done.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3036
src/isolate.cc:3036: factory()->undefined_value(),
On 2016/10/11 23:14:30, adamk wrote:
> Same as the above, I'd hoist this out.

Done.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3037
src/isolate.cc:3037: arraysize(argv), argv, maybe_exception);
On 2016/10/11 23:14:30, adamk wrote:
> I'm not sure the same logic about result and maybe_exception holds as it did
> before. This is only going to remember the last call.

Done.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3092
src/isolate.cc:3092: // TODO(gsathya): remove IsJSFunction?
On 2016/10/11 23:14:30, adamk wrote:
> You'd have to change the V8 API before you could do that, so I don't think
this
> TODO belongs here.

Done.

https://codereview.chromium.org/2406343002/diff/80001/src/isolate.cc#newcode3171
src/isolate.cc:3171: if (microtask->IsPromiseContainer()) {
On 2016/10/11 23:14:30, adamk wrote:
> This should be merged with the else above

Done.

https://codereview.chromium.org/2406343002/diff/80001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2406343002/diff/80001/src/js/promise.js#newco...
src/js/promise.js:193: var id, name, before_debug, after_debug, instrumenting =
DEBUG_IS_ACTIVE;
On 2016/10/11 23:14:30, adamk wrote:
> Nit: s/before_debug/beforeDebug/
> 
> Same for after_debug

Done.

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

https://codereview.chromium.org/2406343002/diff/80001/src/objects-debug.cc#ne...
src/objects-debug.cc:932: reaction_context()->ObjectVerify();
On 2016/10/11 23:14:30, adamk wrote:
> I think I led you down the wrong path here the first time (still waiting for a
> response on v8-dev to be sure). But I think the only checks you want here are
> that fields are of the right type, e.g.,
> 
> CHECK(tasks()->IsJSArray() || tasks()->IsCallable())

Why is tasks special? Don't we want to check if deferred is Undefined or
callable too?

https://codereview.chromium.org/2406343002/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2406343002/diff/80001/src/objects.h#newcode6687
src/objects.h:6687: class PromiseContainer : public Struct {
On 2016/10/11 23:14:30, adamk wrote:
> This name now sounds too generic. Maybe add a TODO to rename it?

Done.

https://codereview.chromium.org/2406343002/diff/80001/src/objects.h#newcode6714
src/objects.h:6714: class PromiseHandleInfo : public Struct {
On 2016/10/11 23:14:30, adamk wrote:
> This name is a little funny; why not PromiseReactionJobInfo?

Done.

https://codereview.chromium.org/2406343002/diff/80001/src/objects.h#newcode6718
src/objects.h:6718: DECL_ACCESSORS(deferred, Object)
On 2016/10/11 23:14:30, adamk wrote:
> Isn't this guaranteed to be a JSObject?

Nope this can be UNDEFINED when we have more than one callback. The comment in
PromiseReactionJob explains this. Happy to add another comment here if you think
that's useful.

https://codereview.chromium.org/2406343002/diff/80001/src/objects.h#newcode6721
src/objects.h:6721: DECL_ACCESSORS(reaction_context, Context)
On 2016/10/11 23:14:30, adamk wrote:
> I think "context" would be a fine name for this.

Done.

[commit-bot: I haz the power, 2016-10-12 02:28:00]

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

[commit-bot: I haz the power, 2016-10-12 02:28:01]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-10-12 16:18:12]

Looking good, almost there. Excited about the perf boost!

https://codereview.chromium.org/2406343002/diff/160001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2406343002/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:3013: class PromiseDebugEventHelper {
We often name these things scopes, so how about PromiseDebugEventScope?

https://codereview.chromium.org/2406343002/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:3015: PromiseDebugEventHelper(Isolate* isolate, Handle<Object>
before,
You could reduce the lines-of-code (or at least encapsulate more of the logic)
by taking Object* as before/after and constructing the handles 1) only if debug
is active and 2) after checking debug_is_active

https://codereview.chromium.org/2406343002/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:3066: if (result->is_null() && maybe_exception->is_null()) {
Copy and paste a comment with this, too:

// If execution is terminating, just bail out.

https://codereview.chromium.org/2406343002/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:3074: Execution::TryCall(this, promise_handle(),
factory()->undefined_value(),
You can pass promise_handle_fn and undefined here

https://codereview.chromium.org/2406343002/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:3076: if (result->is_null() && maybe_exception->is_null()) {
No need to do anything special in this case, right? We're about to return
anyway.

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

https://codereview.chromium.org/2406343002/diff/160001/src/objects-debug.cc#n...
src/objects-debug.cc:927: CHECK(tasks()->IsJSArray() || tasks()->IsCallable());
Aren't there more things you could check here? Basically I'd expect to see
CHECKs about all the expected states of the fields.

https://codereview.chromium.org/2406343002/diff/160001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2406343002/diff/160001/src/objects.h#newcode6714
src/objects.h:6714: // Struct to hold state required for PromiseReactionJobInfo.
Typo: remove "Info" from end of this comment.

https://codereview.chromium.org/2406343002/diff/160001/src/objects.h#newcode6730
src/objects.h:6730: static const int kContext = kAfterDebugEventOffset +
kPointerSize;
s/kContext/kContextOffset/

[gsathya, 2016-10-12 17:25:50]

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

[commit-bot: I haz the power, 2016-10-12 17:25:55]

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-12 17:49:59]

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

[commit-bot: I haz the power, 2016-10-12 17:50:00]

Dry run: Try jobs failed on following builders:
  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_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)

[gsathya, 2016-10-12 19:01:55]

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

[commit-bot: I haz the power, 2016-10-12 19:01:58]

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-12 19:29:47]

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

[commit-bot: I haz the power, 2016-10-12 19:29:47]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-10-12 20:08:36]

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

[commit-bot: I haz the power, 2016-10-12 20:08:48]

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-12 20:08:53]

https://codereview.chromium.org/2406343002/diff/160001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2406343002/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:3013: class PromiseDebugEventHelper {
On 2016/10/12 16:18:11, adamk wrote:
> We often name these things scopes, so how about PromiseDebugEventScope?

Done.

https://codereview.chromium.org/2406343002/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:3015: PromiseDebugEventHelper(Isolate* isolate, Handle<Object>
before,
On 2016/10/12 16:18:11, adamk wrote:
> You could reduce the lines-of-code (or at least encapsulate more of the logic)
> by taking Object* as before/after and constructing the handles 1) only if
debug
> is active and 2) after checking debug_is_active

Done.

https://codereview.chromium.org/2406343002/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:3066: if (result->is_null() && maybe_exception->is_null()) {
On 2016/10/12 16:18:11, adamk wrote:
> Copy and paste a comment with this, too:
> 
> // If execution is terminating, just bail out.

Done.

https://codereview.chromium.org/2406343002/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:3074: Execution::TryCall(this, promise_handle(),
factory()->undefined_value(),
On 2016/10/12 16:18:11, adamk wrote:
> You can pass promise_handle_fn and undefined here

Done.

https://codereview.chromium.org/2406343002/diff/160001/src/isolate.cc#newcode...
src/isolate.cc:3076: if (result->is_null() && maybe_exception->is_null()) {
On 2016/10/12 16:18:11, adamk wrote:
> No need to do anything special in this case, right? We're about to return
> anyway.

Done.

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

https://codereview.chromium.org/2406343002/diff/160001/src/objects-debug.cc#n...
src/objects-debug.cc:927: CHECK(tasks()->IsJSArray() || tasks()->IsCallable());
On 2016/10/12 16:18:11, adamk wrote:
> Aren't there more things you could check here? Basically I'd expect to see
> CHECKs about all the expected states of the fields.

Yeah I was curious about this too. I guess you missed my question in the
previous reply.

Done.

https://codereview.chromium.org/2406343002/diff/160001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2406343002/diff/160001/src/objects.h#newcode6714
src/objects.h:6714: // Struct to hold state required for PromiseReactionJobInfo.
On 2016/10/12 16:18:12, adamk wrote:
> Typo: remove "Info" from end of this comment.

Done.

https://codereview.chromium.org/2406343002/diff/160001/src/objects.h#newcode6730
src/objects.h:6730: static const int kContext = kAfterDebugEventOffset +
kPointerSize;
On 2016/10/12 16:18:12, adamk wrote:
> s/kContext/kContextOffset/

Done.

[adamk, 2016-10-12 20:11:54]

lgtm

[gsathya, 2016-10-12 20:13:20]

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

[gsathya, 2016-10-12 20:13:51]

On 2016/10/12 20:11:54, adamk wrote:
> lgtm

+benedikt for compiler/

[gsathya, 2016-10-12 20:14:34]

Description was changed from

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

This patch results in a 11% improvement over 5 runs in the 
bluebird benchmark.

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

to

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

This patch results in a 11% improvement over 5 runs in the 
bluebird benchmark.

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

[adamk, 2016-10-12 20:16:09]

Description was changed from

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

This patch results in a 11% improvement over 5 runs in the 
bluebird benchmark.

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

to

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

This patch results in a 11% improvement over 5 runs in the 
bluebird benchmark.

BUG=v8:5343,v8:5046
TBR=bmeurer@chromium.org
==========

[adamk, 2016-10-12 20:16:32]

TBRing bmeurer since this is just adding a new instance type.

[commit-bot: I haz the power, 2016-10-12 20:40:59]

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

[commit-bot: I haz the power, 2016-10-12 20:41:00]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-10-12 20:42:57]

The CQ bit was checked by gsathya@chromium.org

[commit-bot: I haz the power, 2016-10-12 20:43:09]

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-12 20:45:35]

Description was changed from

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

This patch results in a 11% improvement over 5 runs in the 
bluebird benchmark.

BUG=v8:5343,v8:5046
TBR=bmeurer@chromium.org
==========

to

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

This patch results in a 11% improvement over 5 runs in the 
bluebird benchmark.

BUG=v8:5343,v8:5046
TBR=bmeurer@chromium.org
==========

[commit-bot: I haz the power, 2016-10-12 20:45:36]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2016-10-12 20:46:07]

Description was changed from

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

This patch results in a 11% improvement over 5 runs in the 
bluebird benchmark.

BUG=v8:5343,v8:5046
TBR=bmeurer@chromium.org
==========

to

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

This patch results in a 11% improvement over 5 runs in the
bluebird benchmark.

BUG=v8:5343,v8:5046
TBR=bmeurer@chromium.org

Committed: https://crrev.com/6f94a8f1c7f0a94c74c5055b02b660d8e93fe5fe
Cr-Commit-Position: refs/heads/master@{#40239}
==========

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

Patchset 12 (id:??) landed as
https://crrev.com/6f94a8f1c7f0a94c74c5055b02b660d8e93fe5fe
Cr-Commit-Position: refs/heads/master@{#40239}

[Benedikt Meurer, 2016-10-13 03:35:14]

Very nice! LGTM