[promises] Remove deferred object

[promises] Remove deferred object

This patch stores the promise, resolve, reject properties of the
deferred object created by CreateInternalPromiseCapability and
NewPromiseCapability directly on the promise (if the promise hasn't
been fulfilled), otherwise they are stored on the
PromiseReactionJobInfo.

This patch removes the currently unused
CreateInternalPromiseCapability and inlines the call to create the
deferred promise object.

NewPromiseCapability is the only function that works with a deferred.

This patch results in a 8.5% improvement in benchmarks over 5 runs.

BUG=v8:5343
Review-Url: https://codereview.chromium.org/2590563003
Cr-Commit-Position: refs/heads/master@{#41991}
Committed: https://chromium.googlesource.com/v8/v8/+/5668ce39873f97f0bd8b25289f41c88cf91aa9bc

[gsathya, 2016-12-20 01:33:16]

Description was changed from

==========
[promises] Remove deferred object

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

to

==========
[promises] Remove deferred object

This patch stores the promise, resolve, reject properties of the
deferred object created by CreateInternalPromiseCapability and
NewPromiseCapability directly on the promise (if the promise hasn't
been fulfilled), otherwise they are stored on the
PromiseReactionJobInfo.

This patch removes the currently unused
CreateInternalPromiseCapability and inlines the call to create the
deferred promise object.

NewPromiseCapability is the only function that works with a deferred.

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

[gsathya, 2016-12-20 01:33:27]

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

[commit-bot: I haz the power, 2016-12-20 01:33:29]

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-20 01:34:35]

Description was changed from

==========
[promises] Remove deferred object

This patch stores the promise, resolve, reject properties of the
deferred object created by CreateInternalPromiseCapability and
NewPromiseCapability directly on the promise (if the promise hasn't
been fulfilled), otherwise they are stored on the
PromiseReactionJobInfo.

This patch removes the currently unused
CreateInternalPromiseCapability and inlines the call to create the
deferred promise object.

NewPromiseCapability is the only function that works with a deferred.

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

to

==========
[promises] Remove deferred object

This patch stores the promise, resolve, reject properties of the
deferred object created by CreateInternalPromiseCapability and
NewPromiseCapability directly on the promise (if the promise hasn't
been fulfilled), otherwise they are stored on the
PromiseReactionJobInfo.

This patch removes the currently unused
CreateInternalPromiseCapability and inlines the call to create the
deferred promise object.

NewPromiseCapability is the only function that works with a deferred.

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

[gsathya, 2016-12-20 01:34:35]

gsathya@chromium.org changed reviewers:
+ ishell@chromium.org, jgruber@chromium.org

[gsathya, 2016-12-20 01:35:54]

Description was changed from

==========
[promises] Remove deferred object

This patch stores the promise, resolve, reject properties of the
deferred object created by CreateInternalPromiseCapability and
NewPromiseCapability directly on the promise (if the promise hasn't
been fulfilled), otherwise they are stored on the
PromiseReactionJobInfo.

This patch removes the currently unused
CreateInternalPromiseCapability and inlines the call to create the
deferred promise object.

NewPromiseCapability is the only function that works with a deferred.

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

to

==========
[promises] Remove deferred object

This patch stores the promise, resolve, reject properties of the
deferred object created by CreateInternalPromiseCapability and
NewPromiseCapability directly on the promise (if the promise hasn't
been fulfilled), otherwise they are stored on the
PromiseReactionJobInfo.

This patch removes the currently unused
CreateInternalPromiseCapability and inlines the call to create the
deferred promise object.

NewPromiseCapability is the only function that works with a deferred.

This patch results in a 8.5% improvement in benchmarks over 5 runs.

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

[gsathya, 2016-12-20 01:40:04]

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

[commit-bot: I haz the power, 2016-12-20 01:40:09]

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-20 01:57:58]

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

[commit-bot: I haz the power, 2016-12-20 01:57:59]

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

[caitp, 2016-12-20 02:53:00]

caitp@igalia.com changed reviewers:
+ caitp@igalia.com

[caitp, 2016-12-20 02:53:01]

https://codereview.chromium.org/2590563003/diff/40001/src/builtins/builtins-p...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2590563003/diff/40001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:316: StoreObjectField(promise,
JSPromise::kDeferredPromiseOffset,
The multiple chains case seems likely to regress when we do this, isn't it? I
realize the assumption is that this isn't the "common" case, but it's not
something that never happens.

[gsathya, 2016-12-20 03:56:24]

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

[commit-bot: I haz the power, 2016-12-20 03:56:29]

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-20 04:15:24]

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

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

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

[gsathya, 2016-12-20 18:18:00]

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

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

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-20 18:38:31]

PTAL, all tests pass.

On 2016/12/20 02:53:01, caitp wrote:
>
https://codereview.chromium.org/2590563003/diff/40001/src/builtins/builtins-p...
> File src/builtins/builtins-promise.cc (right):
> 
>
https://codereview.chromium.org/2590563003/diff/40001/src/builtins/builtins-p...
> src/builtins/builtins-promise.cc:316: StoreObjectField(promise,
> JSPromise::kDeferredPromiseOffset,
> The multiple chains case seems likely to regress when we do this, isn't it? I
> realize the assumption is that this isn't the "common" case, but it's not
> something that never happens.

There isn't a multiple chains case in the benchmark :P I could potentially just
store these 3 items in the same fixedarray and not create a new one for each.
Can you think of anything else?

[commit-bot: I haz the power, 2016-12-20 18:49:03]

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

[commit-bot: I haz the power, 2016-12-20 18:49:04]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-12-21 08:08:29]

Nice! lgtm with comments.

https://codereview.chromium.org/2590563003/diff/80001/src/builtins/builtins-p...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2590563003/diff/80001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:286:
StoreFixedArrayElement(deferred_promise_arr, 1, deferred_promise);
Maybe refactor this into a NewFixedArrayWithElements helper on
PromiseBuiltinsAssembler?

https://codereview.chromium.org/2590563003/diff/80001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:356:
CallRuntime(Runtime::kEnqueuePromiseReactionJob, context, info,
It'd be awesome to move this to CSA as well at some point. Could you add a TODO?

https://codereview.chromium.org/2590563003/diff/80001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:384: PromiseSetHasHandler(promise);
By the way, we could make PromiseSetHasHandler cheaper by not smi-untagging &
tagging, and storing the smi without a write barrier.

https://codereview.chromium.org/2590563003/diff/80001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:953: // Get promise from deferred
Leftover comment?

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

https://codereview.chromium.org/2590563003/diff/80001/src/objects.h#newcode6591
src/objects.h:6591: DECL_ACCESSORS(deferred_promise, Object)
Let's document the tristate (Object/Undefined/FixedArray) of these as well or
point to the comment in JSPromise.

[jgruber, 2016-12-21 08:45:07]

https://codereview.chromium.org/2590563003/diff/80001/src/builtins/builtins-p...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2590563003/diff/80001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:384: PromiseSetHasHandler(promise);
On 2016/12/21 08:08:28, jgruber wrote:
> By the way, we could make PromiseSetHasHandler cheaper by not smi-untagging &
> tagging, and storing the smi without a write barrier.

CL for that here: https://codereview.chromium.org/2591193002/

[gsathya, 2016-12-21 19:05:26]

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

[commit-bot: I haz the power, 2016-12-21 19:05:37]

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-21 19:26:29]

https://codereview.chromium.org/2590563003/diff/80001/src/builtins/builtins-p...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2590563003/diff/80001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:286:
StoreFixedArrayElement(deferred_promise_arr, 1, deferred_promise);
On 2016/12/21 08:08:28, jgruber wrote:
> Maybe refactor this into a NewFixedArrayWithElements helper on
> PromiseBuiltinsAssembler?

This is slightly tricky and doesn't conform together well enough to be
refactored out because I have the existing_deferred_promise already in the first
case but not for the rest.

Leaving this as such for now.

https://codereview.chromium.org/2590563003/diff/80001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:356:
CallRuntime(Runtime::kEnqueuePromiseReactionJob, context, info,
On 2016/12/21 08:08:28, jgruber wrote:
> It'd be awesome to move this to CSA as well at some point. Could you add a
TODO?

Done.

https://codereview.chromium.org/2590563003/diff/80001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:384: PromiseSetHasHandler(promise);
On 2016/12/21 08:45:07, jgruber wrote:
> On 2016/12/21 08:08:28, jgruber wrote:
> > By the way, we could make PromiseSetHasHandler cheaper by not smi-untagging
&
> > tagging, and storing the smi without a write barrier.
> 
> CL for that here: https://codereview.chromium.org/2591193002/

Thanks!

https://codereview.chromium.org/2590563003/diff/80001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:953: // Get promise from deferred
On 2016/12/21 08:08:28, jgruber wrote:
> Leftover comment?

Done.

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

https://codereview.chromium.org/2590563003/diff/80001/src/objects.h#newcode6591
src/objects.h:6591: DECL_ACCESSORS(deferred_promise, Object)
On 2016/12/21 08:08:28, jgruber wrote:
> Let's document the tristate (Object/Undefined/FixedArray) of these as well or
> point to the comment in JSPromise.

Done.

[commit-bot: I haz the power, 2016-12-21 19:56:17]

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

[commit-bot: I haz the power, 2016-12-21 19:56:18]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-22 16:19:40]

Igor, PTAL, thanks!

[Igor Sheludko, 2016-12-28 06:56:45]

lgtm

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

https://codereview.chromium.org/2590563003/diff/100001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:362: AllocateFixedArray(FAST_ELEMENTS,
IntPtrConstant(2));
BTW, we have Tuple2 and Tuple3 structs.

[gsathya, 2016-12-29 19:48:17]

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

https://codereview.chromium.org/2590563003/diff/100001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:362: AllocateFixedArray(FAST_ELEMENTS,
IntPtrConstant(2));
On 2016/12/28 06:56:45, Igor Sheludko wrote:
> BTW, we have Tuple2 and Tuple3 structs.

Hmm, but these can grow to length > 3

[gsathya, 2016-12-29 19:49:27]

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

[commit-bot: I haz the power, 2016-12-29 19:49:35]

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-29 20:17:28]

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

[commit-bot: I haz the power, 2016-12-29 20:17:29]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-29 20:27:55]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2016-12-29 20:27:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
jgruber@chromium.org, ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2590563003/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-29 20:27:59]

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-29 20:30:29]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483043275575780,
"parent_rev": "26c293a93ba4b9d2d0751a336ece556275a560e1", "commit_rev":
"5668ce39873f97f0bd8b25289f41c88cf91aa9bc"}

[commit-bot: I haz the power, 2016-12-29 20:30:34]

Description was changed from

==========
[promises] Remove deferred object

This patch stores the promise, resolve, reject properties of the
deferred object created by CreateInternalPromiseCapability and
NewPromiseCapability directly on the promise (if the promise hasn't
been fulfilled), otherwise they are stored on the
PromiseReactionJobInfo.

This patch removes the currently unused
CreateInternalPromiseCapability and inlines the call to create the
deferred promise object.

NewPromiseCapability is the only function that works with a deferred.

This patch results in a 8.5% improvement in benchmarks over 5 runs.

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

to

==========
[promises] Remove deferred object

This patch stores the promise, resolve, reject properties of the
deferred object created by CreateInternalPromiseCapability and
NewPromiseCapability directly on the promise (if the promise hasn't
been fulfilled), otherwise they are stored on the
PromiseReactionJobInfo.

This patch removes the currently unused
CreateInternalPromiseCapability and inlines the call to create the
deferred promise object.

NewPromiseCapability is the only function that works with a deferred.

This patch results in a 8.5% improvement in benchmarks over 5 runs.

BUG=v8:5343

Review-Url: https://codereview.chromium.org/2590563003
Cr-Commit-Position: refs/heads/master@{#41991}
Committed:
https://chromium.googlesource.com/v8/v8/+/5668ce39873f97f0bd8b25289f41c88cf91...
==========

[commit-bot: I haz the power, 2016-12-29 20:30:35]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/v8/v8/+/5668ce39873f97f0bd8b25289f41c88cf91...