Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2028253004:
Promises: Short circuit promise resolution procedure (Closed)
DescriptionPromises: Short circuit promise resolution procedure
When |ResolvePromise| is resolved with a promise that is
already fulfilled or resolved, we can short circuit the
promise resolution procedure by directly looking up the
result from the promise. We save creating two closures, enqueuing in
the promise queue, and running through PromiseThen.
This patch uses IsPromise to check if the |resolution| object is a native
promise and also checks if |resolution.then| hasn't been monkey
patched.
This patch adds some redundant code from PromiseThen like setting
the promiseHasHandlerSymbol and calling PromiseRevokeReject call,
which would've been taken care of by PromiseThen in the old code path.
This patch results in a 13.8% improvement(over 5 runs) in the bluebird
benchmarks.
BUG=v8:5046
Committed: https://crrev.com/41c875a69e69aeea0c1599d4eb8bf0d03784faac
Cr-Commit-Position: refs/heads/master@{#36765}
Patch Set 1 #Patch Set 2 : fix tests #Messages
Total messages: 17 (9 generated)
Description was changed from ========== Promises: Short circuit promise resolution procedure When ResolvePromise is resolved with a promise that is already fulfilled or resolved, we can short circuit the promise resolution procedure by directly looking up the result from the promise. I saw a 1.19% improvement(over 5 runs) in the bluebird benchmarks. BUG=v8:5046 ========== to ========== Promises: Short circuit promise resolution procedure When |ResolvePromise| is resolved with a promise that is already fulfilled or resolved, we can short circuit the promise resolution procedure by directly looking up the result from the promise. This patch adds a check to see if the |resolution| object is a native promise and also checks if the |resolution.then| has not been changed. I saw a 13.8% improvement(over 5 runs) in the bluebird benchmarks. BUG=v8:5046 ==========
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/patch-status/2028253004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2028253004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Promises: Short circuit promise resolution procedure When |ResolvePromise| is resolved with a promise that is already fulfilled or resolved, we can short circuit the promise resolution procedure by directly looking up the result from the promise. This patch adds a check to see if the |resolution| object is a native promise and also checks if the |resolution.then| has not been changed. I saw a 13.8% improvement(over 5 runs) in the bluebird benchmarks. BUG=v8:5046 ========== to ========== Promises: Short circuit promise resolution procedure When |ResolvePromise| is resolved with a promise that is already fulfilled or resolved, we can short circuit the promise resolution procedure by directly looking up the result from the promise. We save creating two closures, enqueuing in the promise queue, and running through PromiseThen. This patch uses IsPromise to check if the |resolution| object is a native promise and also checks if |resolution.then| hasn't been monkey patched. This patch adds some redundant code from PromiseThen like setting the promiseHasHandlerSymbol and calling PromiseRevokeReject call, which would've been taken care of by PromiseThen in the old code path. This patch results in a 13.8% improvement(over 5 runs) in the bluebird benchmarks. BUG=v8:5046 ==========
gsathya@chromium.org changed reviewers: + adamk@chromium.org, littledan@chromium.org
Description was changed from ========== Promises: Short circuit promise resolution procedure When |ResolvePromise| is resolved with a promise that is already fulfilled or resolved, we can short circuit the promise resolution procedure by directly looking up the result from the promise. We save creating two closures, enqueuing in the promise queue, and running through PromiseThen. This patch uses IsPromise to check if the |resolution| object is a native promise and also checks if |resolution.then| hasn't been monkey patched. This patch adds some redundant code from PromiseThen like setting the promiseHasHandlerSymbol and calling PromiseRevokeReject call, which would've been taken care of by PromiseThen in the old code path. This patch results in a 13.8% improvement(over 5 runs) in the bluebird benchmarks. BUG=v8:5046 ========== to ========== Promises: Short circuit promise resolution procedure When |ResolvePromise| is resolved with a promise that is already fulfilled or resolved, we can short circuit the promise resolution procedure by directly looking up the result from the promise. We save creating two closures, enqueuing in the promise queue, and running through PromiseThen. This patch uses IsPromise to check if the |resolution| object is a native promise and also checks if |resolution.then| hasn't been monkey patched. This patch adds some redundant code from PromiseThen like setting the promiseHasHandlerSymbol and calling PromiseRevokeReject call, which would've been taken care of by PromiseThen in the old code path. This patch results in a 13.8% improvement(over 5 runs) in the bluebird benchmarks. BUG=v8:5046 ==========
lgtm
Seems correct because nothing could've happened in those future microtask queue
items except call then, which would've done the same things--nothing in there
would've been calling out to user code. If the microtask queue is fairly busy,
it might lead to things being processed a bit sooner to not take the extra
trips, but I think this is acceptable (maybe the spec should be rephrased a
little to make it clear that this is OK).
This patch appears to handle a case like this in async functions:
async function foo() { }
async function bar() { return foo() }
(as opposed to `return await foo()`)
I have no idea how common this is in real code. If ES Promises had gone with
more C#-like chain semantics, the optimization would be unnecessary, as it would
clearly return a Promise of a Promise of undefined. This pattern will surely
come up in real code sometimes, though I don't know whether impact on a real
site would be so high as on the Bluebird suite. This points to the need for
further Promise benchmarks, as you've mentioned.
lgtm, though I think we should be careful about over-fitting to this benchmark (agree with both of your comments about wanting more benchmarks here).
The CQ bit was checked by gsathya@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2028253004/20001
Message was sent while issue was closed.
Description was changed from ========== Promises: Short circuit promise resolution procedure When |ResolvePromise| is resolved with a promise that is already fulfilled or resolved, we can short circuit the promise resolution procedure by directly looking up the result from the promise. We save creating two closures, enqueuing in the promise queue, and running through PromiseThen. This patch uses IsPromise to check if the |resolution| object is a native promise and also checks if |resolution.then| hasn't been monkey patched. This patch adds some redundant code from PromiseThen like setting the promiseHasHandlerSymbol and calling PromiseRevokeReject call, which would've been taken care of by PromiseThen in the old code path. This patch results in a 13.8% improvement(over 5 runs) in the bluebird benchmarks. BUG=v8:5046 ========== to ========== Promises: Short circuit promise resolution procedure When |ResolvePromise| is resolved with a promise that is already fulfilled or resolved, we can short circuit the promise resolution procedure by directly looking up the result from the promise. We save creating two closures, enqueuing in the promise queue, and running through PromiseThen. This patch uses IsPromise to check if the |resolution| object is a native promise and also checks if |resolution.then| hasn't been monkey patched. This patch adds some redundant code from PromiseThen like setting the promiseHasHandlerSymbol and calling PromiseRevokeReject call, which would've been taken care of by PromiseThen in the old code path. This patch results in a 13.8% improvement(over 5 runs) in the bluebird benchmarks. BUG=v8:5046 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Promises: Short circuit promise resolution procedure When |ResolvePromise| is resolved with a promise that is already fulfilled or resolved, we can short circuit the promise resolution procedure by directly looking up the result from the promise. We save creating two closures, enqueuing in the promise queue, and running through PromiseThen. This patch uses IsPromise to check if the |resolution| object is a native promise and also checks if |resolution.then| hasn't been monkey patched. This patch adds some redundant code from PromiseThen like setting the promiseHasHandlerSymbol and calling PromiseRevokeReject call, which would've been taken care of by PromiseThen in the old code path. This patch results in a 13.8% improvement(over 5 runs) in the bluebird benchmarks. BUG=v8:5046 ========== to ========== Promises: Short circuit promise resolution procedure When |ResolvePromise| is resolved with a promise that is already fulfilled or resolved, we can short circuit the promise resolution procedure by directly looking up the result from the promise. We save creating two closures, enqueuing in the promise queue, and running through PromiseThen. This patch uses IsPromise to check if the |resolution| object is a native promise and also checks if |resolution.then| hasn't been monkey patched. This patch adds some redundant code from PromiseThen like setting the promiseHasHandlerSymbol and calling PromiseRevokeReject call, which would've been taken care of by PromiseThen in the old code path. This patch results in a 13.8% improvement(over 5 runs) in the bluebird benchmarks. BUG=v8:5046 Committed: https://crrev.com/41c875a69e69aeea0c1599d4eb8bf0d03784faac Cr-Commit-Position: refs/heads/master@{#36765} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/41c875a69e69aeea0c1599d4eb8bf0d03784faac Cr-Commit-Position: refs/heads/master@{#36765} |
