[promises] Port ResolvePromise to TF

[promises] Port ResolvePromise to TF

-- Moves promiseHasHandlerSymbol to inobject property
-- Ports PromiseResolveClosure to TF
-- Fix a non spec async-await test which fails now because we do a map
check for native promise check (instead of IsPromise). Changing the
constructor (in the test) invalidates the map check.

This patch results in a 7.1% performance improvement in the bluebird
benchmark (over 5 runs).

BUG=v8:5343
Committed: https://crrev.com/11359e331ae98978fe91b6c6925a1569dbd61856
Cr-Commit-Position: refs/heads/master@{#41569}

[gsathya, 2016-12-02 03:20:34]

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

[commit-bot: I haz the power, 2016-12-02 03:20:45]

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-02 03:25:34]

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

[commit-bot: I haz the power, 2016-12-02 03:25:35]

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/18776)

[gsathya, 2016-12-02 03:44:42]

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

[commit-bot: I haz the power, 2016-12-02 03:44:43]

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-02 03:47:54]

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

[commit-bot: I haz the power, 2016-12-02 03:48:03]

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-02 04:04:47]

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

[commit-bot: I haz the power, 2016-12-02 04:04:48]

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

Description was changed from

==========
[promises] Port ResolvePromise to TF

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

to

==========
[promises] Port ResolvePromise to TF

-- Ports PromiseResolveClosure to TF
-- Deletes non spec async-await spec which fails now because we do a map check
for native promise check (instead of IsPromise). Changing the constructor (in
the test) invalidates the map check.

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

[gsathya, 2016-12-03 00:02:23]

gsathya@chromium.org changed reviewers:
+ bmeurer@chromium.org, caitp@igalia.com, jgruber@chromium.org

[gsathya, 2016-12-03 00:08:58]

Description was changed from

==========
[promises] Port ResolvePromise to TF

-- Ports PromiseResolveClosure to TF
-- Deletes non spec async-await spec which fails now because we do a map check
for native promise check (instead of IsPromise). Changing the constructor (in
the test) invalidates the map check.

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

to

==========
[promises] Port ResolvePromise to TF

-- Ports PromiseResolveClosure to TF
-- Deletes non spec async-await spec which fails now because we do a map 
check for native promise check (instead of IsPromise). Changing the 
constructor (in the test) invalidates the map check.

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

[gsathya, 2016-12-03 01:44:00]

Description was changed from

==========
[promises] Port ResolvePromise to TF

-- Ports PromiseResolveClosure to TF
-- Deletes non spec async-await spec which fails now because we do a map 
check for native promise check (instead of IsPromise). Changing the 
constructor (in the test) invalidates the map check.

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

to

==========
[promises] Port ResolvePromise to TF

-- Ports PromiseResolveClosure to TF
-- Deletes non spec async-await spec which fails now because we do a map 
check for native promise check (instead of IsPromise). Changing the 
constructor (in the test) invalidates the map check.

This patch results in a 7.1% performance improvement in the bluebird 
benchmark (over 5 runs).

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

[caitp, 2016-12-03 02:44:16]

Until we can enqueue microtasks from JS, doesn't it make more sense for these to
just be implemented in c++?

[gsathya, 2016-12-03 03:04:40]

On 2016/12/03 02:44:16, caitp wrote:
> Until we can enqueue microtasks from JS, doesn't it make more sense for these
to
> just be implemented in c++?

I assume you mean TF, not JS? I can port them all in the same patch I guess, but
that's a lot of code and this isn't a regression. I'd rather do it piecemeal
unless there's a regression and avoid porting JS-> C++  and then C++ -> TF 
again.

[caitp, 2016-12-03 03:43:02]

On 2016/12/03 03:04:40, gsathya wrote:
> On 2016/12/03 02:44:16, caitp wrote:
> > Until we can enqueue microtasks from JS, doesn't it make more sense for
these
> to
> > just be implemented in c++?
> 
> I assume you mean TF, not JS? I can port them all in the same patch I guess,
but
> that's a lot of code and this isn't a regression. I'd rather do it piecemeal
> unless there's a regression and avoid porting JS-> C++  and then C++ -> TF 
> again.

Ok, I just wanted to understand the architectural direction this is headed in.
Sounds reasonable to me.

[jgruber, 2016-12-05 09:25:17]

lgtm

lgtm with comments

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

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:645: // RegExp fast path implementations rely
on unmodified JSPromise instances.
Promise

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:673: // TODO(ishell): Update this check once
map changes for constant field
We can remove this TODO since the mentioned changes will not be happening AFAIK
(I should also do the same in builtins-regexp).

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:681: typedef CodeStubAssembler::Variable
Variable;
As mentioned offline, you might consider switching to the new TF_BUILTIN style
sooner than later. You won't need these typedefs in every function in that case.

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:693: a->GotoIf(a->WordEqual(promise, result),
&if_cycle);
SameValue is more involved than WordEqual. You can use CSA::SameValue.

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:698:
a->GotoUnless(a->IsJSReceiverInstanceType(result_instance_type), &fulfill);
IsJSReceiver()

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:822: a.GotoIf(a.SmiEqual(has_already_visited,
a.SmiConstant(1)), &out);
Is line 825 below the only place where we write into kAlreadyVisitedSlot? Would
it make sense to store True/FalseConstant instead of 0/1 Smis?

https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc...
src/js/promise.js:121: %resolve_promise(promise, resolution);
Can we replace calls to ResolvePromise by %resolve_promise and remove
ResolvePromise?

https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc...
src/js/promise.js:405: %SetCode(GlobalPromise.prototype.catch, PromiseCatch);
This and below + corresponding changes in bootstrapper seem unrelated?

[gsathya, 2016-12-05 16:03:05]

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

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:645: // RegExp fast path implementations rely
on unmodified JSPromise instances.
On 2016/12/05 09:25:17, jgruber wrote:
> Promise

Done.

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:673: // TODO(ishell): Update this check once
map changes for constant field
On 2016/12/05 09:25:17, jgruber wrote:
> We can remove this TODO since the mentioned changes will not be happening
AFAIK
> (I should also do the same in builtins-regexp).

Done.

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:681: typedef CodeStubAssembler::Variable
Variable;
On 2016/12/05 09:25:17, jgruber wrote:
> As mentioned offline, you might consider switching to the new TF_BUILTIN style
> sooner than later. You won't need these typedefs in every function in that
case.

I'm going to wait until I finish the rest of the builtins before doing the
cleanup. I currently have 4 levels of dependent patches so I don't want to deal
with too much unnecessary churn right now.

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:693: a->GotoIf(a->WordEqual(promise, result),
&if_cycle);
On 2016/12/05 09:25:17, jgruber wrote:
> SameValue is more involved than WordEqual. You can use CSA::SameValue.

Done.

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:698:
a->GotoUnless(a->IsJSReceiverInstanceType(result_instance_type), &fulfill);
On 2016/12/05 09:25:17, jgruber wrote:
> IsJSReceiver()

Done.

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:822: a.GotoIf(a.SmiEqual(has_already_visited,
a.SmiConstant(1)), &out);
On 2016/12/05 09:25:17, jgruber wrote:
> Is line 825 below the only place where we write into kAlreadyVisitedSlot?
Would
> it make sense to store True/FalseConstant instead of 0/1 Smis?

PromiseUtils::CreateResolvingFunctions and PromiseRejectClosure also use this. I
think Smi is cheaper though anyway

https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc...
src/js/promise.js:121: %resolve_promise(promise, resolution);
On 2016/12/05 09:25:17, jgruber wrote:
> Can we replace calls to ResolvePromise by %resolve_promise and remove
> ResolvePromise?

Nope. We use this in async-await and v8-extras

https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc...
src/js/promise.js:405: %SetCode(GlobalPromise.prototype.catch, PromiseCatch);
On 2016/12/05 09:25:17, jgruber wrote:
> This and below + corresponding changes in bootstrapper seem unrelated?

I need to install an empty .catch in bootstrapper and set the code here,
otherwise the prototype will change here and my map checks will always fail.

[caitp, 2016-12-05 16:22:51]

https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc...
src/js/promise.js:121: %resolve_promise(promise, resolution);
On 2016/12/05 16:03:05, gsathya wrote:
> On 2016/12/05 09:25:17, jgruber wrote:
> > Can we replace calls to ResolvePromise by %resolve_promise and remove
> > ResolvePromise?
> 
> Nope. We use this in async-await and v8-extras

Couldn't we just export %resolve_promise from the bootstrapper and avoid
creating the extra closure in promise.js?

[gsathya, 2016-12-06 03:08:58]

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

[commit-bot: I haz the power, 2016-12-06 03:09: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-12-06 03:15:13]

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

[commit-bot: I haz the power, 2016-12-06 03:15:14]

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

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

[commit-bot: I haz the power, 2016-12-06 06:02:05]

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-06 06:27:35]

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

[commit-bot: I haz the power, 2016-12-06 06:27:36]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-06 19:02:51]

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

[commit-bot: I haz the power, 2016-12-06 19:03: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-12-06 19:08:32]

Description was changed from

==========
[promises] Port ResolvePromise to TF

-- Ports PromiseResolveClosure to TF
-- Deletes non spec async-await spec which fails now because we do a map 
check for native promise check (instead of IsPromise). Changing the 
constructor (in the test) invalidates the map check.

This patch results in a 7.1% performance improvement in the bluebird 
benchmark (over 5 runs).

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

to

==========
[promises] Port ResolvePromise to TF

-- Moves promiseHasHandlerSymbol to inobject property
-- Ports PromiseResolveClosure to TF
-- Deletes non spec async-await spec which fails now because we do a map 
check for native promise check (instead of IsPromise). Changing the 
constructor (in the test) invalidates the map check.

This patch results in a 7.1% performance improvement in the bluebird 
benchmark (over 5 runs).

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

[gsathya, 2016-12-06 19:10:48]

On 2016/12/05 16:22:51, caitp wrote:
> https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js
> File src/js/promise.js (right):
> 
>
https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc...
> src/js/promise.js:121: %resolve_promise(promise, resolution);
> On 2016/12/05 16:03:05, gsathya wrote:
> > On 2016/12/05 09:25:17, jgruber wrote:
> > > Can we replace calls to ResolvePromise by %resolve_promise and remove
> > > ResolvePromise?
> > 
> > Nope. We use this in async-await and v8-extras
> 
> Couldn't we just export %resolve_promise from the bootstrapper and avoid
> creating the extra closure in promise.js?

Done.

PTAL

[caitp, 2016-12-06 19:16:58]

On 2016/12/06 19:10:48, gsathya wrote:
> On 2016/12/05 16:22:51, caitp wrote:
> > https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js
> > File src/js/promise.js (right):
> > 
> >
>
https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc...
> > src/js/promise.js:121: %resolve_promise(promise, resolution);
> > On 2016/12/05 16:03:05, gsathya wrote:
> > > On 2016/12/05 09:25:17, jgruber wrote:
> > > > Can we replace calls to ResolvePromise by %resolve_promise and remove
> > > > ResolvePromise?
> > > 
> > > Nope. We use this in async-await and v8-extras
> > 
> > Couldn't we just export %resolve_promise from the bootstrapper and avoid
> > creating the extra closure in promise.js?
> 
> Done.
> 
> PTAL

Removal of ResolvePromise lgtm

[gsathya, 2016-12-06 19:28:16]

On 2016/12/06 19:16:58, caitp wrote:
> On 2016/12/06 19:10:48, gsathya wrote:
> > On 2016/12/05 16:22:51, caitp wrote:
> > > https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js
> > > File src/js/promise.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc...
> > > src/js/promise.js:121: %resolve_promise(promise, resolution);
> > > On 2016/12/05 16:03:05, gsathya wrote:
> > > > On 2016/12/05 09:25:17, jgruber wrote:
> > > > > Can we replace calls to ResolvePromise by %resolve_promise and remove
> > > > > ResolvePromise?
> > > > 
> > > > Nope. We use this in async-await and v8-extras
> > > 
> > > Couldn't we just export %resolve_promise from the bootstrapper and avoid
> > > creating the extra closure in promise.js?
> > 
> > Done.
> > 
> > PTAL
> 
> Removal of ResolvePromise lgtm

Thanks caitp!

benedikt, jgruber, the CL has changed a fair bit, can you PTAL?

[commit-bot: I haz the power, 2016-12-06 19:32:25]

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

[commit-bot: I haz the power, 2016-12-06 19:32:26]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-06 21:29:44]

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

[gsathya, 2016-12-06 21:30:06]

+ishell

[jgruber, 2016-12-07 12:59:39]

Nit: s/spec/test/ in commit message.

Nice! lgtm modulo comments.

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

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:681: typedef CodeStubAssembler::Variable
Variable;
On 2016/12/05 16:03:05, gsathya wrote:
> On 2016/12/05 09:25:17, jgruber wrote:
> > As mentioned offline, you might consider switching to the new TF_BUILTIN
style
> > sooner than later. You won't need these typedefs in every function in that
> case.
> 
> I'm going to wait until I finish the rest of the builtins before doing the
> cleanup. I currently have 4 levels of dependent patches so I don't want to
deal
> with too much unnecessary churn right now.

Acknowledged, sgtm.

https://codereview.chromium.org/2541283002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:822: a.GotoIf(a.SmiEqual(has_already_visited,
a.SmiConstant(1)), &out);
On 2016/12/05 16:03:05, gsathya wrote:
> On 2016/12/05 09:25:17, jgruber wrote:
> > Is line 825 below the only place where we write into kAlreadyVisitedSlot?
> Would
> > it make sense to store True/FalseConstant instead of 0/1 Smis?
> 
> PromiseUtils::CreateResolvingFunctions and PromiseRejectClosure also use this.
I
> think Smi is cheaper though anyway

Ok. IMHO the fact that we use a Smi 0/1 encoding is not obvious and we should at
least document this well. Perhaps in promise-utils.h together with the contents
of all other custom slots? It seems kind of off though that only Promise uses
Smi 0/1 to represent true/false. Likewise for JSPromise::kHasHandlerOffset. If
you do decide to change it, a follow-up CL is also fine.

And out of interest, are Smis really cheaper? It should be a pointer comparison
against a constant in both cases.

https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2541283002/diff/140001/src/js/promise.js#newc...
src/js/promise.js:405: %SetCode(GlobalPromise.prototype.catch, PromiseCatch);
On 2016/12/05 16:03:05, gsathya wrote:
> On 2016/12/05 09:25:17, jgruber wrote:
> > This and below + corresponding changes in bootstrapper seem unrelated?
> 
> I need to install an empty .catch in bootstrapper and set the code here,
> otherwise the prototype will change here and my map checks will always fail.

Acknowledged.

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

https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:360: Node* new_elements =
a->AllocateFixedArray(kind, new_capacity, mode);
Do you think we need to handle LO space allocation here as well?

https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:669: CodeStubAssembler::Label* out) {
I don't think you gain much through the style

IRP(.., out) { Goto(out); }
caller() {
 IRP(...)
 Bind(&out)
 Return(UndefinedConstant());
}

vs. the simpler

IRP(...) { Bind(&out); return UndefinedConstant(); }
caller() {
 Return(IRP(...));
}

or

IRP(...) { Bind(&out); }
caller() {
 IRP(...);
 Return(UndefinedConstant());
}

But I don't feel too strongly about this.

https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:779: a->GotoUnless(is_promise, &enqueue);
This will always evaluate to true (even if you pass in FalseConstant).
Branch/Gotos interpret IntPtrConstant(0) as false and everything else as true.

Just skip the SelectBooleanConstant call.

https://codereview.chromium.org/2541283002/diff/240001/src/js/async-await.js
File src/js/async-await.js (right):

https://codereview.chromium.org/2541283002/diff/240001/src/js/async-await.js#...
src/js/async-await.js:89: %PromiseMarkAsHandled(promise);
Should this be %PromiseMarkAsHandled(throwawayCapability.promise)?

https://codereview.chromium.org/2541283002/diff/240001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2541283002/diff/240001/src/objects.h#newcode8916
src/objects.h:8916: DECL_INT_ACCESSORS(has_handler)
Please add a comment for content meaning and possible values.

BTW, you can use the DECL_BOOLEAN_ACCESSORS convenience functions if you haven't
seen them yet.

https://codereview.chromium.org/2541283002/diff/240001/src/promise-utils.h
File src/promise-utils.h (right):

https://codereview.chromium.org/2541283002/diff/240001/src/promise-utils.h#ne...
src/promise-utils.h:17: enum PromiseResolvingFunctionContextSlot {
Please document these (if not already done elsewhere).

Thinking about this some more, are there any situations where we will conflict
with other code by blindly storing into specific context slots?

[Igor Sheludko, 2016-12-07 16:23:52]

lgtm with a nit:

https://codereview.chromium.org/2541283002/diff/240001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2541283002/diff/240001/src/api.cc#newcode7243
src/api.cc:7243: return js_promise->has_handler() == 1;
Suggestion: != 0 would probably be nicer here. Or a named constant.

https://codereview.chromium.org/2541283002/diff/240001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2541283002/diff/240001/src/objects.h#newcode8916
src/objects.h:8916: DECL_INT_ACCESSORS(has_handler)
On 2016/12/07 12:59:38, jgruber wrote:
> Please add a comment for content meaning and possible values.
> 
> BTW, you can use the DECL_BOOLEAN_ACCESSORS convenience functions if you
haven't
> seen them yet.

+1. Please turn this into smi "flag" field + boolean "has_handler" accessor as
Jakob suggested. It will make caller code look nicer.

[Dan Ehrenberg, 2016-12-07 23:24:58]

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

[Dan Ehrenberg, 2016-12-07 23:24:58]

https://codereview.chromium.org/2541283002/diff/240001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-await-no-constructor.js (left):

https://codereview.chromium.org/2541283002/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-await-no-constructor.js:27: assertEquals(0, count);
Rather than deleting this test, would it be possible to change it to check for
the right behavior that this code sample should've done? This would form a
regression test against the spec violation that existed before. This would just
be changing 0 to the observed value.

[gsathya, 2016-12-08 01:27:17]

Description was changed from

==========
[promises] Port ResolvePromise to TF

-- Moves promiseHasHandlerSymbol to inobject property
-- Ports PromiseResolveClosure to TF
-- Deletes non spec async-await spec which fails now because we do a map 
check for native promise check (instead of IsPromise). Changing the 
constructor (in the test) invalidates the map check.

This patch results in a 7.1% performance improvement in the bluebird 
benchmark (over 5 runs).

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

to

==========
[promises] Port ResolvePromise to TF

-- Moves promiseHasHandlerSymbol to inobject property
-- Ports PromiseResolveClosure to TF
-- Deletes non spec async-await test which fails now because we do a map 
check for native promise check (instead of IsPromise). Changing the 
constructor (in the test) invalidates the map check.

This patch results in a 7.1% performance improvement in the bluebird 
benchmark (over 5 runs).

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

[gsathya, 2016-12-08 04:01:57]

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

[commit-bot: I haz the power, 2016-12-08 04:02:03]

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-08 04:09:43]

Description was changed from

==========
[promises] Port ResolvePromise to TF

-- Moves promiseHasHandlerSymbol to inobject property
-- Ports PromiseResolveClosure to TF
-- Deletes non spec async-await test which fails now because we do a map 
check for native promise check (instead of IsPromise). Changing the 
constructor (in the test) invalidates the map check.

This patch results in a 7.1% performance improvement in the bluebird 
benchmark (over 5 runs).

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

to

==========
[promises] Port ResolvePromise to TF

-- Moves promiseHasHandlerSymbol to inobject property
-- Ports PromiseResolveClosure to TF
-- Fix a non spec async-await test which fails now because we do a map 
check for native promise check (instead of IsPromise). Changing the 
constructor (in the test) invalidates the map check.

This patch results in a 7.1% performance improvement in the bluebird 
benchmark (over 5 runs).

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

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

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

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

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-12-08 04:57:22]

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

[commit-bot: I haz the power, 2016-12-08 04:57:31]

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-08 05:18:15]

https://codereview.chromium.org/2541283002/diff/240001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2541283002/diff/240001/src/api.cc#newcode7243
src/api.cc:7243: return js_promise->has_handler() == 1;
On 2016/12/07 16:23:52, Igor Sheludko wrote:
> Suggestion: != 0 would probably be nicer here. Or a named constant.

Done.

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

https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:360: Node* new_elements =
a->AllocateFixedArray(kind, new_capacity, mode);
On 2016/12/07 12:59:38, jgruber wrote:
> Do you think we need to handle LO space allocation here as well?

This is handled in https://codereview.chromium.org/2556483002/

https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:669: CodeStubAssembler::Label* out) {
On 2016/12/07 12:59:38, jgruber wrote:
> I don't think you gain much through the style
> 
> IRP(.., out) { Goto(out); }
> caller() {
>  IRP(...)
>  Bind(&out)
>  Return(UndefinedConstant());
> }
> 
> vs. the simpler
> 
> IRP(...) { Bind(&out); return UndefinedConstant(); }
> caller() {
>  Return(IRP(...));
> }
> 
> or
> 
> IRP(...) { Bind(&out); }
> caller() {
>  IRP(...);
>  Return(UndefinedConstant());
> }
> 
> But I don't feel too strongly about this.

I use this here in the follow on patch --
https://codereview.chromium.org/2561443003/diff/60001/src/builtins/builtins-p...

https://codereview.chromium.org/2541283002/diff/240001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:779: a->GotoUnless(is_promise, &enqueue);
On 2016/12/07 12:59:38, jgruber wrote:
> This will always evaluate to true (even if you pass in FalseConstant).
> Branch/Gotos interpret IntPtrConstant(0) as false and everything else as true.
> 
> Just skip the SelectBooleanConstant call.

Done.

https://codereview.chromium.org/2541283002/diff/240001/src/js/async-await.js
File src/js/async-await.js (right):

https://codereview.chromium.org/2541283002/diff/240001/src/js/async-await.js#...
src/js/async-await.js:89: %PromiseMarkAsHandled(promise);
On 2016/12/07 12:59:38, jgruber wrote:
> Should this be %PromiseMarkAsHandled(throwawayCapability.promise)?

Yes.

https://codereview.chromium.org/2541283002/diff/240001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2541283002/diff/240001/src/objects.h#newcode8916
src/objects.h:8916: DECL_INT_ACCESSORS(has_handler)
On 2016/12/07 16:23:52, Igor Sheludko wrote:
> On 2016/12/07 12:59:38, jgruber wrote:
> > Please add a comment for content meaning and possible values.
> > 
> > BTW, you can use the DECL_BOOLEAN_ACCESSORS convenience functions if you
> haven't
> > seen them yet.
> 
> +1. Please turn this into smi "flag" field + boolean "has_handler" accessor as
> Jakob suggested. It will make caller code look nicer.

Done.

https://codereview.chromium.org/2541283002/diff/240001/src/promise-utils.h
File src/promise-utils.h (right):

https://codereview.chromium.org/2541283002/diff/240001/src/promise-utils.h#ne...
src/promise-utils.h:17: enum PromiseResolvingFunctionContextSlot {
On 2016/12/07 12:59:38, jgruber wrote:
> Please document these (if not already done elsewhere).

Done.

> Thinking about this some more, are there any situations where we will conflict
> with other code by blindly storing into specific context slots? 

These are unused slots, so it's fine

https://codereview.chromium.org/2541283002/diff/240001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-await-no-constructor.js (left):

https://codereview.chromium.org/2541283002/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-await-no-constructor.js:27: assertEquals(0, count);
On 2016/12/07 23:24:58, Dan Ehrenberg wrote:
> Rather than deleting this test, would it be possible to change it to check for
> the right behavior that this code sample should've done? This would form a
> regression test against the spec violation that existed before. This would
just
> be changing 0 to the observed value.

Done

[Benedikt Meurer, 2016-12-08 05:22:30]

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

https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:286: namespace {
Nit: empty line after namespace {

https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:295: void PromiseSetHandler(CodeStubAssembler*
a, compiler::Node* promise) {
Nit: PromiseSetHasHandler

https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:299: Node* const new_flags = a->WordOr(
You can perform this operation on the Smi directly.

https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:654: namespace {
Nit: whitespace after { and before }

https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:687: void
InternalResolvePromise(CodeStubAssembler* a, compiler::Node* context,
Nit: move into the namespace above.

[gsathya, 2016-12-08 05:27:08]

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

[commit-bot: I haz the power, 2016-12-08 05:27: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-12-08 05:28:40]

Thanks for the reviews!

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

https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:286: namespace {
On 2016/12/08 05:22:30, Benedikt Meurer wrote:
> Nit: empty line after namespace {

Done.

https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:295: void PromiseSetHandler(CodeStubAssembler*
a, compiler::Node* promise) {
On 2016/12/08 05:22:30, Benedikt Meurer wrote:
> Nit: PromiseSetHasHandler

Done.

https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:299: Node* const new_flags = a->WordOr(
On 2016/12/08 05:22:30, Benedikt Meurer wrote:
> You can perform this operation on the Smi directly.

Done.

https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:654: namespace {
On 2016/12/08 05:22:30, Benedikt Meurer wrote:
> Nit: whitespace after { and before }

Done.

https://codereview.chromium.org/2541283002/diff/280001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:687: void
InternalResolvePromise(CodeStubAssembler* a, compiler::Node* context,
On 2016/12/08 05:22:30, Benedikt Meurer wrote:
> Nit: move into the namespace above.

Done.

[commit-bot: I haz the power, 2016-12-08 05:51:04]

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

[commit-bot: I haz the power, 2016-12-08 05:51:05]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-08 06:10:33]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2016-12-08 06:10:34]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
caitp@igalia.com, jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2541283002/#ps300001
(title: "fix nits")

[commit-bot: I haz the power, 2016-12-08 06:10:38]

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-08 06:12:36]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1481177433659920,
"parent_rev": "03fe0bea54cfbb9e96800bc82c29064b5ac94ede", "commit_rev":
"11f689acfaf2db80a355e4d6660ae7bd3b96c0e0"}

[commit-bot: I haz the power, 2016-12-08 06:12:40]

Description was changed from

==========
[promises] Port ResolvePromise to TF

-- Moves promiseHasHandlerSymbol to inobject property
-- Ports PromiseResolveClosure to TF
-- Fix a non spec async-await test which fails now because we do a map 
check for native promise check (instead of IsPromise). Changing the 
constructor (in the test) invalidates the map check.

This patch results in a 7.1% performance improvement in the bluebird 
benchmark (over 5 runs).

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

to

==========
[promises] Port ResolvePromise to TF

-- Moves promiseHasHandlerSymbol to inobject property
-- Ports PromiseResolveClosure to TF
-- Fix a non spec async-await test which fails now because we do a map 
check for native promise check (instead of IsPromise). Changing the 
constructor (in the test) invalidates the map check.

This patch results in a 7.1% performance improvement in the bluebird 
benchmark (over 5 runs).

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

[commit-bot: I haz the power, 2016-12-08 06:12:41]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-12-08 06:12:54]

Description was changed from

==========
[promises] Port ResolvePromise to TF

-- Moves promiseHasHandlerSymbol to inobject property
-- Ports PromiseResolveClosure to TF
-- Fix a non spec async-await test which fails now because we do a map 
check for native promise check (instead of IsPromise). Changing the 
constructor (in the test) invalidates the map check.

This patch results in a 7.1% performance improvement in the bluebird 
benchmark (over 5 runs).

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

to

==========
[promises] Port ResolvePromise to TF

-- Moves promiseHasHandlerSymbol to inobject property
-- Ports PromiseResolveClosure to TF
-- Fix a non spec async-await test which fails now because we do a map
check for native promise check (instead of IsPromise). Changing the
constructor (in the test) invalidates the map check.

This patch results in a 7.1% performance improvement in the bluebird
benchmark (over 5 runs).

BUG=v8:5343

Committed: https://crrev.com/11359e331ae98978fe91b6c6925a1569dbd61856
Cr-Commit-Position: refs/heads/master@{#41569}
==========

[commit-bot: I haz the power, 2016-12-08 06:12:56]

Patchset 16 (id:??) landed as
https://crrev.com/11359e331ae98978fe91b6c6925a1569dbd61856
Cr-Commit-Position: refs/heads/master@{#41569}

[jochen (gone - plz use gerrit), 2016-12-08 15:56:07]

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

[jochen (gone - plz use gerrit), 2016-12-08 15:56:08]

https://codereview.chromium.org/2541283002/diff/300001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2541283002/diff/300001/src/js/promise.js#newc...
src/js/promise.js:391: "promise_catch", PromiseCatch,
fyi, after the %SetCode above, we shouldn't use the original method anymore..
this will crash in the future