Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396763002:
[promises] dont create resolving closures in PromiseThen (Closed)
Description[promises] dont create resolving closures in PromiseThen
When we create native promises as part of PromiseThen, we don't have
to create resolving closures. The closure will only ever be called
once from PromiseHandle, therefore we don't need the alreadyResolved
check.
This results in a 21.76% improvement in the bluebird benchmark
over 5 runs.
BUG=v8:5046
Committed: https://crrev.com/4dc97f4a83fc2bb4193d6cc1779dd0763e0d5f4e
Cr-Commit-Position: refs/heads/master@{#40018}
Patch Set 1 #Patch Set 2 : modify NewPromiseCapability #Patch Set 3 : add comment back #
Total comments: 1
Patch Set 4 : revert stuff #Patch Set 5 : fix broken revert #
Total comments: 10
Patch Set 6 : address review comments #
Total comments: 1
Patch Set 7 : Add todo #Messages
Total messages: 33 (21 generated)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [promises] dont create resolving closures in PromiseThen When we create native promises as part of PromiseThen, we don't have to create resolving closures. The closure will only ever be called once from PromiseHandle, therefore we don't need the alreadyResolved check. BUG=v8:5046 ========== to ========== [promises] dont create resolving closures in PromiseThen When we create native promises as part of PromiseThen, we don't have to create resolving closures. The closure will only ever be called once from PromiseHandle, therefore we don't need the alreadyResolved check. This results in a 21.76% improvement in the bluebird benchmark over 5 runs. BUG=v8:5046 ==========
Description was changed from ========== [promises] dont create resolving closures in PromiseThen When we create native promises as part of PromiseThen, we don't have to create resolving closures. The closure will only ever be called once from PromiseHandle, therefore we don't need the alreadyResolved check. This results in a 21.76% improvement in the bluebird benchmark over 5 runs. BUG=v8:5046 ========== to ========== [promises] dont create resolving closures in PromiseThen When we create native promises as part of PromiseThen, we don't have to create resolving closures. The closure will only ever be called once from PromiseHandle, therefore we don't need the alreadyResolved check. This results in a 21.76% improvement in the bluebird benchmark over 5 runs. BUG=v8:5046 ==========
gsathya@chromium.org changed reviewers: + adamk@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This looks awesome. As discussed offline, it seems like NewPromiseCapability should just have its fast path improved for all callers.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [promises] dont create resolving closures in PromiseThen When we create native promises as part of PromiseThen, we don't have to create resolving closures. The closure will only ever be called once from PromiseHandle, therefore we don't need the alreadyResolved check. This results in a 21.76% improvement in the bluebird benchmark over 5 runs. BUG=v8:5046 ========== to ========== [promises] dont create resolving closures in PromiseThen When we create native promises as part of PromiseThen, we don't have to create resolving closures. The closure will only ever be called once from PromiseHandle, therefore we don't need the alreadyResolved check. This CL also changes CreateResolveElementFunction to not return an arrow function. This results in a 21.76% improvement in the bluebird benchmark over 5 runs. BUG=v8:5046 ==========
On 2016/10/05 18:18:34, adamk wrote: > This looks awesome. As discussed offline, it seems like NewPromiseCapability > should just have its fast path improved for all callers. PTAL, some of the callers need the callbacks so I have some amount of duplicated code. Not sure if this is strictly better.
https://codereview.chromium.org/2396763002/diff/40001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2396763002/diff/40001/src/js/promise.js#newco... src/js/promise.js:570: var callbacks = CreateResolvingFunctions(deferred.promise, false); Ah, I didn't realize some callers need the callbacks. If that's the case then I agree this is worse.
Description was changed from ========== [promises] dont create resolving closures in PromiseThen When we create native promises as part of PromiseThen, we don't have to create resolving closures. The closure will only ever be called once from PromiseHandle, therefore we don't need the alreadyResolved check. This CL also changes CreateResolveElementFunction to not return an arrow function. This results in a 21.76% improvement in the bluebird benchmark over 5 runs. BUG=v8:5046 ========== to ========== [promises] dont create resolving closures in PromiseThen When we create native promises as part of PromiseThen, we don't have to create resolving closures. The closure will only ever be called once from PromiseHandle, therefore we don't need the alreadyResolved check. This results in a 21.76% improvement in the bluebird benchmark over 5 runs. BUG=v8:5046 ==========
On 2016/10/05 20:18:09, adamk wrote: > https://codereview.chromium.org/2396763002/diff/40001/src/js/promise.js > File src/js/promise.js (right): > > https://codereview.chromium.org/2396763002/diff/40001/src/js/promise.js#newco... > src/js/promise.js:570: var callbacks = > CreateResolvingFunctions(deferred.promise, false); > Ah, I didn't realize some callers need the callbacks. If that's the case then I > agree this is worse. Reverted
On 2016/10/05 20:18:09, adamk wrote: > https://codereview.chromium.org/2396763002/diff/40001/src/js/promise.js > File src/js/promise.js (right): > > https://codereview.chromium.org/2396763002/diff/40001/src/js/promise.js#newco... > src/js/promise.js:570: var callbacks = > CreateResolvingFunctions(deferred.promise, false); > Ah, I didn't realize some callers need the callbacks. If that's the case then I > agree this is worse. Reverted
https://codereview.chromium.org/2396763002/diff/80001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2396763002/diff/80001/src/js/promise.js#newco... src/js/promise.js:172: if (deferred.resolve) { deferred.resolve(result); } And an explicit IS_UNDEFINED() check for these, just to make the boolean stuff more explicit. https://codereview.chromium.org/2396763002/diff/80001/src/js/promise.js#newco... src/js/promise.js:173: else { ResolvePromise(deferred.promise, result); } Please reformat this and the below into the usual if { } else { } style https://codereview.chromium.org/2396763002/diff/80001/src/js/promise.js#newco... src/js/promise.js:177: else { RejectPromise(deferred.promise, exception, false); } This needs a comment for why false is the third argument; maybe just a copy of the comment in PromiseThen. https://codereview.chromium.org/2396763002/diff/80001/src/js/promise.js#newco... src/js/promise.js:461: resultCapability = { Please add a comment here explaining this special case. https://codereview.chromium.org/2396763002/diff/80001/src/js/promise.js#newco... src/js/promise.js:464: reject: false How about UNDEFINED for these falses...
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2396763002/diff/80001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2396763002/diff/80001/src/js/promise.js#newco... src/js/promise.js:172: if (deferred.resolve) { deferred.resolve(result); } On 2016/10/05 22:12:00, adamk wrote: > And an explicit IS_UNDEFINED() check for these, just to make the boolean stuff > more explicit. Done. https://codereview.chromium.org/2396763002/diff/80001/src/js/promise.js#newco... src/js/promise.js:173: else { ResolvePromise(deferred.promise, result); } On 2016/10/05 22:12:00, adamk wrote: > Please reformat this and the below into the usual > > if { > } else { > } > > style Done. https://codereview.chromium.org/2396763002/diff/80001/src/js/promise.js#newco... src/js/promise.js:177: else { RejectPromise(deferred.promise, exception, false); } On 2016/10/05 22:12:00, adamk wrote: > This needs a comment for why false is the third argument; maybe just a copy of > the comment in PromiseThen. Done. https://codereview.chromium.org/2396763002/diff/80001/src/js/promise.js#newco... src/js/promise.js:461: resultCapability = { On 2016/10/05 22:12:00, adamk wrote: > Please add a comment here explaining this special case. Done. https://codereview.chromium.org/2396763002/diff/80001/src/js/promise.js#newco... src/js/promise.js:464: reject: false On 2016/10/05 22:12:00, adamk wrote: > How about UNDEFINED for these falses... Done.
lgtm! https://codereview.chromium.org/2396763002/diff/100001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2396763002/diff/100001/src/js/promise.js#newc... src/js/promise.js:473: resultCapability = { Can you add a TODO for a followup patch to combine this into NewPromiseCapability? I don't want to block the 20% improvement on that refactor though.
The CQ bit was checked by gsathya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2396763002/#ps2 (title: "Add todo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [promises] dont create resolving closures in PromiseThen When we create native promises as part of PromiseThen, we don't have to create resolving closures. The closure will only ever be called once from PromiseHandle, therefore we don't need the alreadyResolved check. This results in a 21.76% improvement in the bluebird benchmark over 5 runs. BUG=v8:5046 ========== to ========== [promises] dont create resolving closures in PromiseThen When we create native promises as part of PromiseThen, we don't have to create resolving closures. The closure will only ever be called once from PromiseHandle, therefore we don't need the alreadyResolved check. This results in a 21.76% improvement in the bluebird benchmark over 5 runs. BUG=v8:5046 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:2)
Message was sent while issue was closed.
Description was changed from ========== [promises] dont create resolving closures in PromiseThen When we create native promises as part of PromiseThen, we don't have to create resolving closures. The closure will only ever be called once from PromiseHandle, therefore we don't need the alreadyResolved check. This results in a 21.76% improvement in the bluebird benchmark over 5 runs. BUG=v8:5046 ========== to ========== [promises] dont create resolving closures in PromiseThen When we create native promises as part of PromiseThen, we don't have to create resolving closures. The closure will only ever be called once from PromiseHandle, therefore we don't need the alreadyResolved check. This results in a 21.76% improvement in the bluebird benchmark over 5 runs. BUG=v8:5046 Committed: https://crrev.com/4dc97f4a83fc2bb4193d6cc1779dd0763e0d5f4e Cr-Commit-Position: refs/heads/master@{#40018} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4dc97f4a83fc2bb4193d6cc1779dd0763e0d5f4e Cr-Commit-Position: refs/heads/master@{#40018} |
