[async-await] Don't create resolving callbacks for throwaway promises

[async-await] Don't create resolving callbacks for throwaway promises

This patch also cleans up NewPromiseCapability.

This patch results in a 20% improvement over 4 runs with the following micro
benchmark -

var x = Promise.resolve();

async function bar() {
    return x;
}

async function foo() {
    await bar();
}

var start = performance.now();
var count = 0;
var max = 10000;
for(var i = 0; i <= max; i++) {
    foo().then(() => {
        count++;
        if(count === max) print( performance.now() - start );
    })
}

BUG=v8:5639
Committed: https://crrev.com/764548e2cd80f8f50b2c62cc944a183fc7779af6
Cr-Commit-Position: refs/heads/master@{#41116}

[gsathya, 2016-11-18 04:56:38]

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

[commit-bot: I haz the power, 2016-11-18 04:56:39]

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

[gsathya, 2016-11-18 05:17:52]

Description was changed from

==========
[async-await] dont'create resolving callbacks for throwaway promises

This patch also cleans up NewPromiseCapability

BUG=v8:5639
==========

to

==========
[async-await] dont'create resolving callbacks for throwaway promises

This patch also cleans up NewPromiseCapability.

This patch results in a 20% improvement over 4 runs with following
microbenchmark -

var x = Promise.resolve();

async function bar() {
    return x;
}

async function foo() {
    await bar();
}

var start = performance.now();
var count = 0;
var max = 10000;
for(var i = 0; i <= max; i++) {
    foo().then(() => {
        count++;
        if(count === max) print( performance.now() - start );
    })
}

BUG=v8:5639
==========

[gsathya, 2016-11-18 05:18:39]

Description was changed from

==========
[async-await] dont'create resolving callbacks for throwaway promises

This patch also cleans up NewPromiseCapability.

This patch results in a 20% improvement over 4 runs with following
microbenchmark -

var x = Promise.resolve();

async function bar() {
    return x;
}

async function foo() {
    await bar();
}

var start = performance.now();
var count = 0;
var max = 10000;
for(var i = 0; i <= max; i++) {
    foo().then(() => {
        count++;
        if(count === max) print( performance.now() - start );
    })
}

BUG=v8:5639
==========

to

==========
[async-await] dont'create resolving callbacks for throwaway promises

This patch also cleans up NewPromiseCapability.

This patch results in a 20% improvement over 4 runs with the following micro 
benchmark -

var x = Promise.resolve();

async function bar() {
    return x;
}

async function foo() {
    await bar();
}

var start = performance.now();
var count = 0;
var max = 10000;
for(var i = 0; i <= max; i++) {
    foo().then(() => {
        count++;
        if(count === max) print( performance.now() - start );
    })
}

BUG=v8:5639
==========

[commit-bot: I haz the power, 2016-11-18 05:21:28]

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

[commit-bot: I haz the power, 2016-11-18 05:21:29]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-11-18 05:23:57]

Description was changed from

==========
[async-await] dont'create resolving callbacks for throwaway promises

This patch also cleans up NewPromiseCapability.

This patch results in a 20% improvement over 4 runs with the following micro 
benchmark -

var x = Promise.resolve();

async function bar() {
    return x;
}

async function foo() {
    await bar();
}

var start = performance.now();
var count = 0;
var max = 10000;
for(var i = 0; i <= max; i++) {
    foo().then(() => {
        count++;
        if(count === max) print( performance.now() - start );
    })
}

BUG=v8:5639
==========

to

==========
[async-await] Don't create resolving callbacks for throwaway promises

This patch also cleans up NewPromiseCapability.

This patch results in a 20% improvement over 4 runs with the following micro 
benchmark -

var x = Promise.resolve();

async function bar() {
    return x;
}

async function foo() {
    await bar();
}

var start = performance.now();
var count = 0;
var max = 10000;
for(var i = 0; i <= max; i++) {
    foo().then(() => {
        count++;
        if(count === max) print( performance.now() - start );
    })
}

BUG=v8:5639
==========

[gsathya, 2016-11-18 05:25:44]

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

[caitp, 2016-11-18 05:47:15]

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js#newcode303
src/js/promise.js:303: // calling them multiple times.
I like that this is all documented so nicely. Since this behaviour is specific
to await expressions, I think we will want to move this out of
NewPromiseCapability, maybe to a code-stub (or just inlined),  since this
internal-ness is always known statically. The documentation in this CL should
help make that transition easier.

LGTM, but would like assurance that it's not visibly different in the debugger

[gsathya, 2016-11-18 06:02:28]

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js#newcode303
src/js/promise.js:303: // calling them multiple times.
On 2016/11/18 05:47:15, caitp wrote:
> I like that this is all documented so nicely. Since this behaviour is specific
> to await expressions, I think we will want to move this out of
> NewPromiseCapability, maybe to a code-stub (or just inlined),  since this
> internal-ness is always known statically. The documentation in this CL should
> help make that transition easier.

This is used by PromiseThen as well which is why I didn't inline it.
 
> LGTM, but would like assurance that it's not visibly different in the debugger

There shouldn't be anything visibly different in the debugger since its the same
callbacks (without the alreadyResolved check) that's being run.

[Dan Ehrenberg, 2016-11-18 08:05:38]

Great perf improvement!

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js#newcode296
src/js/promise.js:296: function NewPromiseCapability(C, debugEvent,
doNotCreateCallbacks) {
Style suggestion: Rather than making this extra boolean flag (which I think
makes the code harder to read), what if you refactored the true case into a
separate function that either returns PromiseCreate() or falls back to
NewPromiseCapability().promise for other constructors?

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js#newcode303
src/js/promise.js:303: // calling them multiple times.
On 2016/11/18 at 06:02:28, gsathya wrote:
> On 2016/11/18 05:47:15, caitp wrote:
> > I like that this is all documented so nicely. Since this behaviour is
specific
> > to await expressions, I think we will want to move this out of
> > NewPromiseCapability, maybe to a code-stub (or just inlined),  since this
> > internal-ness is always known statically. The documentation in this CL
should
> > help make that transition easier.
> 
> This is used by PromiseThen as well which is why I didn't inline it.

Could you mention this in the commit message? Does this benchmark change
Bluebird perf?
>  
> > LGTM, but would like assurance that it's not visibly different in the
debugger
> 
> There shouldn't be anything visibly different in the debugger since its the
same callbacks (without the alreadyResolved check) that's being run.

This is a decent reason for making it a separate function. The new function does
not need to take debugEvent as an argument, as that argument is just used for
making the callbacks do the right thing.

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js#newcode399
src/js/promise.js:399: constructor, false, constructor === GlobalPromise);
Not sure why you do this comparison. Seems like avoiding creating the functions
is only done on GlobalPromise anyway.

[gsathya, 2016-11-18 08:34:28]

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js#newcode296
src/js/promise.js:296: function NewPromiseCapability(C, debugEvent,
doNotCreateCallbacks) {
On 2016/11/18 08:05:38, Dan Ehrenberg wrote:
> Style suggestion: Rather than making this extra boolean flag (which I think
> makes the code harder to read), what if you refactored the true case into a
> separate function that either returns PromiseCreate() or falls back to
> NewPromiseCapability().promise for other constructors?

Do you mean a function like --
NewPromiseCapabilityWithoutCallbacks() { return { promise: PromiseCreate(),
resolve: UNDEFINED, reject: UNDEFINED} }

This would make code in PromiseThen look a little more verbose --
if (constructor === GlobalPromise) {
  resultCapability = NewPromiseCapabilityWithoutCallbacks();
} else {
  resultCapability = NewPromiseCapability(constructor, false);
}

Do you think this is better?

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js#newcode303
src/js/promise.js:303: // calling them multiple times.
On 2016/11/18 08:05:38, Dan Ehrenberg wrote:
> On 2016/11/18 at 06:02:28, gsathya wrote:
> > On 2016/11/18 05:47:15, caitp wrote:
> > > I like that this is all documented so nicely. Since this behaviour is
> specific
> > > to await expressions, I think we will want to move this out of
> > > NewPromiseCapability, maybe to a code-stub (or just inlined),  since this
> > > internal-ness is always known statically. The documentation in this CL
> should
> > > help make that transition easier.
> > 
> > This is used by PromiseThen as well which is why I didn't inline it.
> 
> Could you mention this in the commit message? Does this benchmark change
> Bluebird perf?

This isn't a new change. This piece of code was specific to PromiseThen, I
refactored it out to be in NewPromiseCapability.

> >  
> > > LGTM, but would like assurance that it's not visibly different in the
> debugger
> > 
> > There shouldn't be anything visibly different in the debugger since its the
> same callbacks (without the alreadyResolved check) that's being run.
> 
> This is a decent reason for making it a separate function. The new function
does
> not need to take debugEvent as an argument, as that argument is just used for
> making the callbacks do the right thing.

Yeah, agreed, but it makes code more verbose in other places as mentioned above.

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js#newcode399
src/js/promise.js:399: constructor, false, constructor === GlobalPromise);
On 2016/11/18 08:05:38, Dan Ehrenberg wrote:
> Not sure why you do this comparison. Seems like avoiding creating the
functions
> is only done on GlobalPromise anyway.

Yes, but this comparison is required to not create the functions. You could
create functions in the the GlobalPromise case too (like in Promise.all and
Promise.race).

[Dan Ehrenberg, 2016-11-18 08:55:37]

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js#newcode296
src/js/promise.js:296: function NewPromiseCapability(C, debugEvent,
doNotCreateCallbacks) {
On 2016/11/18 at 08:34:28, gsathya wrote:
> On 2016/11/18 08:05:38, Dan Ehrenberg wrote:
> > Style suggestion: Rather than making this extra boolean flag (which I think
> > makes the code harder to read), what if you refactored the true case into a
> > separate function that either returns PromiseCreate() or falls back to
> > NewPromiseCapability().promise for other constructors?
> 
> Do you mean a function like --
> NewPromiseCapabilityWithoutCallbacks() { return { promise: PromiseCreate(),
resolve: UNDEFINED, reject: UNDEFINED} }
> 
> This would make code in PromiseThen look a little more verbose --
> if (constructor === GlobalPromise) {
>   resultCapability = NewPromiseCapabilityWithoutCallbacks();
> } else {
>   resultCapability = NewPromiseCapability(constructor, false);
> }
> 
> Do you think this is better?

I was imagining that all of that would be part of the body of
NewPromiseCapabilityWithoutCallbacks. In general, boolean flags are really
difficult for readability/maintainability, and I think it's worth it to add a
tiny bit more verbose code to avoid them.

https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js#newcode399
src/js/promise.js:399: constructor, false, constructor === GlobalPromise);
On 2016/11/18 at 08:34:27, gsathya wrote:
> On 2016/11/18 08:05:38, Dan Ehrenberg wrote:
> > Not sure why you do this comparison. Seems like avoiding creating the
functions
> > is only done on GlobalPromise anyway.
> 
> Yes, but this comparison is required to not create the functions. You could
create functions in the the GlobalPromise case too (like in Promise.all and
Promise.race).

I'd suggest factoring all this logic into NewPromiseCapabilityWithoutCallbacks.
If C is not GlobalPromise, create the functions. The logic only applies to the
callers of this special variant, so it wouldn't affect Promise.all.

[gsathya, 2016-11-18 10:03:30]

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

[commit-bot: I haz the power, 2016-11-18 10:03:40]

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

[gsathya, 2016-11-18 10:05:40]

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

[commit-bot: I haz the power, 2016-11-18 10:05: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-11-18 10:07:08]

On 2016/11/18 08:55:37, Dan Ehrenberg wrote:
> https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js
> File src/js/promise.js (right):
> 
> https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js#newcode296
> src/js/promise.js:296: function NewPromiseCapability(C, debugEvent,
> doNotCreateCallbacks) {
> On 2016/11/18 at 08:34:28, gsathya wrote:
> > On 2016/11/18 08:05:38, Dan Ehrenberg wrote:
> > > Style suggestion: Rather than making this extra boolean flag (which I
think
> > > makes the code harder to read), what if you refactored the true case into
a
> > > separate function that either returns PromiseCreate() or falls back to
> > > NewPromiseCapability().promise for other constructors?
> > 
> > Do you mean a function like --
> > NewPromiseCapabilityWithoutCallbacks() { return { promise: PromiseCreate(),
> resolve: UNDEFINED, reject: UNDEFINED} }
> > 
> > This would make code in PromiseThen look a little more verbose --
> > if (constructor === GlobalPromise) {
> >   resultCapability = NewPromiseCapabilityWithoutCallbacks();
> > } else {
> >   resultCapability = NewPromiseCapability(constructor, false);
> > }
> > 
> > Do you think this is better?
> 
> I was imagining that all of that would be part of the body of
> NewPromiseCapabilityWithoutCallbacks. In general, boolean flags are really
> difficult for readability/maintainability, and I think it's worth it to add a
> tiny bit more verbose code to avoid them.
> 
> https://codereview.chromium.org/2512103002/diff/1/src/js/promise.js#newcode399
> src/js/promise.js:399: constructor, false, constructor === GlobalPromise);
> On 2016/11/18 at 08:34:27, gsathya wrote:
> > On 2016/11/18 08:05:38, Dan Ehrenberg wrote:
> > > Not sure why you do this comparison. Seems like avoiding creating the
> functions
> > > is only done on GlobalPromise anyway.
> > 
> > Yes, but this comparison is required to not create the functions. You could
> create functions in the the GlobalPromise case too (like in Promise.all and
> Promise.race).
> 
> I'd suggest factoring all this logic into
NewPromiseCapabilityWithoutCallbacks.
> If C is not GlobalPromise, create the functions. The logic only applies to the
> callers of this special variant, so it wouldn't affect Promise.all.

PTAL

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

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

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

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-11-18 12:54:52]

lgtm

https://codereview.chromium.org/2512103002/diff/40001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2512103002/diff/40001/src/js/promise.js#newco...
src/js/promise.js:135: // Pass false for debugEvent so .then chaining does not
trigger
Note: this undefined case also includes async/await. False is appropriate for
the same reason, but maybe the comment should be updated.

https://codereview.chromium.org/2512103002/diff/40001/src/js/promise.js#newco...
src/js/promise.js:297: function NewPromiseCapabilityWithoutCallbacks() {
Name suggestion: CreatePromiseCapability

https://codereview.chromium.org/2512103002/diff/40001/src/js/promise.js#newco...
src/js/promise.js:402: 
Nit: consider reverting the irrelevant whitespace change

[gsathya, 2016-11-18 15:39:49]

Thanks for the reviews!

https://codereview.chromium.org/2512103002/diff/40001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2512103002/diff/40001/src/js/promise.js#newco...
src/js/promise.js:135: // Pass false for debugEvent so .then chaining does not
trigger
On 2016/11/18 12:54:52, Dan Ehrenberg wrote:
> Note: this undefined case also includes async/await. False is appropriate for
> the same reason, but maybe the comment should be updated.

Done.

https://codereview.chromium.org/2512103002/diff/40001/src/js/promise.js#newco...
src/js/promise.js:297: function NewPromiseCapabilityWithoutCallbacks() {
On 2016/11/18 12:54:52, Dan Ehrenberg wrote:
> Name suggestion: CreatePromiseCapability

Doesn't seem to convey the difference between NewPromiseCapability and this. I'd
rather leave it as such as it's descriptive.

https://codereview.chromium.org/2512103002/diff/40001/src/js/promise.js#newco...
src/js/promise.js:402: 
On 2016/11/18 12:54:52, Dan Ehrenberg wrote:
> Nit: consider reverting the irrelevant whitespace change

Done.

[gsathya, 2016-11-18 15:39:54]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2016-11-18 15:39:55]

The patchset sent to the CQ was uploaded after l-g-t-m from caitp@igalia.com,
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2512103002/#ps60001
(title: "fix comment")

[commit-bot: I haz the power, 2016-11-18 15:40:00]

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

[caitp, 2016-11-18 15:42:29]

https://codereview.chromium.org/2512103002/diff/40001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2512103002/diff/40001/src/js/promise.js#newco...
src/js/promise.js:297: function NewPromiseCapabilityWithoutCallbacks() {
On 2016/11/18 15:39:48, gsathya wrote:
> On 2016/11/18 12:54:52, Dan Ehrenberg wrote:
> > Name suggestion: CreatePromiseCapability
> 
> Doesn't seem to convey the difference between NewPromiseCapability and this.
I'd
> rather leave it as such as it's descriptive.

"CreateInternalPromiseCapability"?

[gsathya, 2016-11-18 15:45:25]

The CQ bit was unchecked by gsathya@chromium.org

[gsathya, 2016-11-18 15:50:15]

On 2016/11/18 15:42:29, caitp wrote:
> https://codereview.chromium.org/2512103002/diff/40001/src/js/promise.js
> File src/js/promise.js (right):
> 
>
https://codereview.chromium.org/2512103002/diff/40001/src/js/promise.js#newco...
> src/js/promise.js:297: function NewPromiseCapabilityWithoutCallbacks() {
> On 2016/11/18 15:39:48, gsathya wrote:
> > On 2016/11/18 12:54:52, Dan Ehrenberg wrote:
> > > Name suggestion: CreatePromiseCapability
> > 
> > Doesn't seem to convey the difference between NewPromiseCapability and this.
> I'd
> > rather leave it as such as it's descriptive.
> 
> "CreateInternalPromiseCapability"?

SGTM. Updated.

[gsathya, 2016-11-18 15:50:23]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2016-11-18 15:50:24]

The patchset sent to the CQ was uploaded after l-g-t-m from caitp@igalia.com,
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2512103002/#ps80001
(title: "rename to CreateInternalPromiseCapability")

[commit-bot: I haz the power, 2016-11-18 15:50:30]

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-11-18 16:14:59]

Description was changed from

==========
[async-await] Don't create resolving callbacks for throwaway promises

This patch also cleans up NewPromiseCapability.

This patch results in a 20% improvement over 4 runs with the following micro 
benchmark -

var x = Promise.resolve();

async function bar() {
    return x;
}

async function foo() {
    await bar();
}

var start = performance.now();
var count = 0;
var max = 10000;
for(var i = 0; i <= max; i++) {
    foo().then(() => {
        count++;
        if(count === max) print( performance.now() - start );
    })
}

BUG=v8:5639
==========

to

==========
[async-await] Don't create resolving callbacks for throwaway promises

This patch also cleans up NewPromiseCapability.

This patch results in a 20% improvement over 4 runs with the following micro 
benchmark -

var x = Promise.resolve();

async function bar() {
    return x;
}

async function foo() {
    await bar();
}

var start = performance.now();
var count = 0;
var max = 10000;
for(var i = 0; i <= max; i++) {
    foo().then(() => {
        count++;
        if(count === max) print( performance.now() - start );
    })
}

BUG=v8:5639
==========

[commit-bot: I haz the power, 2016-11-18 16:15:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-18 16:15:27]

Description was changed from

==========
[async-await] Don't create resolving callbacks for throwaway promises

This patch also cleans up NewPromiseCapability.

This patch results in a 20% improvement over 4 runs with the following micro 
benchmark -

var x = Promise.resolve();

async function bar() {
    return x;
}

async function foo() {
    await bar();
}

var start = performance.now();
var count = 0;
var max = 10000;
for(var i = 0; i <= max; i++) {
    foo().then(() => {
        count++;
        if(count === max) print( performance.now() - start );
    })
}

BUG=v8:5639
==========

to

==========
[async-await] Don't create resolving callbacks for throwaway promises

This patch also cleans up NewPromiseCapability.

This patch results in a 20% improvement over 4 runs with the following micro
benchmark -

var x = Promise.resolve();

async function bar() {
    return x;
}

async function foo() {
    await bar();
}

var start = performance.now();
var count = 0;
var max = 10000;
for(var i = 0; i <= max; i++) {
    foo().then(() => {
        count++;
        if(count === max) print( performance.now() - start );
    })
}

BUG=v8:5639

Committed: https://crrev.com/764548e2cd80f8f50b2c62cc944a183fc7779af6
Cr-Commit-Position: refs/heads/master@{#41116}
==========

[commit-bot: I haz the power, 2016-11-18 16:15:29]

Patchset 5 (id:??) landed as
https://crrev.com/764548e2cd80f8f50b2c62cc944a183fc7779af6
Cr-Commit-Position: refs/heads/master@{#41116}