Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2577393002:
[js-perf-test] Add a basic async-await microbenchmark (Closed)
Description[js-perf-test] Add a basic async-await microbenchmark
These benchmarks are intended to compare the overhead of async-await vs.
a naive promise implementation vs. the babel async-await transformation.
The functions in the benchmark don't do any work themselves, so results
should reflect only overhead of the chosen implementation.
Current numbers on my local machine (higher is better):
BaselineES2017-AsyncAwait(Score): 2006
BaselineNaivePromises-AsyncAwait(Score): 7470
Native-AsyncAwait(Score): 3640
BUG=v8:5639
Review-Url: https://codereview.chromium.org/2577393002
Cr-Commit-Position: refs/heads/master@{#41860}
Committed: https://chromium.googlesource.com/v8/v8/+/9feefafa66cdfcc3bdb90d9308d028cedfe22916
Patch Set 1 #Patch Set 2 : Add a function with multiple awaits #
Total comments: 8
Patch Set 3 : Address comments #Patch Set 4 : Unroll loop #Patch Set 5 : Re-babel #
Total comments: 2
Messages
Total messages: 38 (28 generated)
The CQ bit was checked by jgruber@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.
The CQ bit was checked by jgruber@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 ========== [js-perf-test] Add a basic async-await microbenchmark These benchmarks are intended to compare the overhead of async-await vs. a naive promise implementation vs. the babel async-await transformation. The functions in the benchmark don't do any work themselves, so results should reflect only overhead of the chosen implementation. Current numbers on my local machine (higher is better): BaselineES2017-AsyncAwait(Score): 2209 BaselineNaivePromises-AsyncAwait(Score): 12478 Native-AsyncAwait(Score): 5990 BUG=v8:5639 ========== to ========== [js-perf-test] Add a basic async-await microbenchmark These benchmarks are intended to compare the overhead of async-await vs. a naive promise implementation vs. the babel async-await transformation. The functions in the benchmark don't do any work themselves, so results should reflect only overhead of the chosen implementation. Current numbers on my local machine (higher is better): BaselineES2017-AsyncAwait(Score): 1805 BaselineNaivePromises-AsyncAwait(Score): 4639 Native-AsyncAwait(Score): 3456 BUG=v8:5639 ==========
jgruber@chromium.org changed reviewers: + gsathya@chromium.org, littledan@chromium.org
This is an initial go at a (very simple) microbenchmark to evaluate and track async/await performance against other alternatives. CCing a couple of people more familiar with the async/await and promises implementation than me. WTYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nice, looking forward to these async function benchmarks! https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... File test/js-perf-test/AsyncAwait/baseline-naive-promises.js (right): https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... test/js-perf-test/AsyncAwait/baseline-naive-promises.js:19: return Promise.all(promises); promises is an array of undefineds, not an array of promises which is different from the async await code https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... File test/js-perf-test/AsyncAwait/native.js (right): https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... test/js-perf-test/AsyncAwait/native.js:17: for (let i = 0; i < 9; i++) await j(); This isn't semantically equivalent to the promise code. await j() waits until the promise j() is resolved before the next iteration starts which isn't true for Promise.all https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... test/js-perf-test/AsyncAwait/native.js:27: a = async function a() { return await b(); }; No need for any await statements here. "return await i()" is same as "return i()" in async functions https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... test/js-perf-test/AsyncAwait/native.js:28: i would rewrite this as async function a { await i(); await h(); await h(); .... }
Great to have this test, modulo Sathya's comments.
The CQ bit was checked by jgruber@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/2577393002/diff/20001/test/js-perf-test/Async... File test/js-perf-test/AsyncAwait/baseline-naive-promises.js (right): https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... test/js-perf-test/AsyncAwait/baseline-naive-promises.js:19: return Promise.all(promises); On 2016/12/16 15:23:05, gsathya wrote: > promises is an array of undefineds, not an array of promises which is different > from the async await code Good catch, done. https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... File test/js-perf-test/AsyncAwait/native.js (right): https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... test/js-perf-test/AsyncAwait/native.js:17: for (let i = 0; i < 9; i++) await j(); On 2016/12/16 15:23:05, gsathya wrote: > This isn't semantically equivalent to the promise code. await j() waits until > the promise j() is resolved before the next iteration starts which isn't true > for Promise.all Done, removed Promise.all. https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... test/js-perf-test/AsyncAwait/native.js:27: a = async function a() { return await b(); }; On 2016/12/16 15:23:05, gsathya wrote: > No need for any await statements here. "return await i()" is same as "return > i()" in async functions Done. https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... test/js-perf-test/AsyncAwait/native.js:28: On 2016/12/16 15:23:05, gsathya wrote: > i would rewrite this as > > async function a { > await i(); > await h(); > await h(); > .... > } The intention was to have both function nesting (a->b->...->j) and functions containing several async calls (i->j,j,...,j). Does that make sense?
Description was changed from ========== [js-perf-test] Add a basic async-await microbenchmark These benchmarks are intended to compare the overhead of async-await vs. a naive promise implementation vs. the babel async-await transformation. The functions in the benchmark don't do any work themselves, so results should reflect only overhead of the chosen implementation. Current numbers on my local machine (higher is better): BaselineES2017-AsyncAwait(Score): 1805 BaselineNaivePromises-AsyncAwait(Score): 4639 Native-AsyncAwait(Score): 3456 BUG=v8:5639 ========== to ========== [js-perf-test] Add a basic async-await microbenchmark These benchmarks are intended to compare the overhead of async-await vs. a naive promise implementation vs. the babel async-await transformation. The functions in the benchmark don't do any work themselves, so results should reflect only overhead of the chosen implementation. Current numbers on my local machine (higher is better): BaselineES2017-AsyncAwait(Score): 1826 BaselineNaivePromises-AsyncAwait(Score): 7330 Native-AsyncAwait(Score): 3770 BUG=v8:5639 ==========
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 jgruber@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.
On 2016/12/19 08:31:02, jgruber wrote: > https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... > File test/js-perf-test/AsyncAwait/baseline-naive-promises.js (right): > > https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... > test/js-perf-test/AsyncAwait/baseline-naive-promises.js:19: return > Promise.all(promises); > On 2016/12/16 15:23:05, gsathya wrote: > > promises is an array of undefineds, not an array of promises which is > different > > from the async await code > > Good catch, done. > > https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... > File test/js-perf-test/AsyncAwait/native.js (right): > > https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... > test/js-perf-test/AsyncAwait/native.js:17: for (let i = 0; i < 9; i++) await > j(); > On 2016/12/16 15:23:05, gsathya wrote: > > This isn't semantically equivalent to the promise code. await j() waits until > > the promise j() is resolved before the next iteration starts which isn't true > > for Promise.all > > Done, removed Promise.all. > > https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... > test/js-perf-test/AsyncAwait/native.js:27: a = async function a() { return await > b(); }; > On 2016/12/16 15:23:05, gsathya wrote: > > No need for any await statements here. "return await i()" is same as "return > > i()" in async functions > > Done. > > https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... > test/js-perf-test/AsyncAwait/native.js:28: > On 2016/12/16 15:23:05, gsathya wrote: > > i would rewrite this as > > > > async function a { > > await i(); > > await h(); > > await h(); > > .... > > } > > The intention was to have both function nesting (a->b->...->j) and functions > containing several async calls (i->j,j,...,j). Does that make sense? Should the promise benchmark be h().then(() => f().then(() => ..) ? this seems same as as single nesting, just that more closures are created. lgtm
Description was changed from ========== [js-perf-test] Add a basic async-await microbenchmark These benchmarks are intended to compare the overhead of async-await vs. a naive promise implementation vs. the babel async-await transformation. The functions in the benchmark don't do any work themselves, so results should reflect only overhead of the chosen implementation. Current numbers on my local machine (higher is better): BaselineES2017-AsyncAwait(Score): 1826 BaselineNaivePromises-AsyncAwait(Score): 7330 Native-AsyncAwait(Score): 3770 BUG=v8:5639 ========== to ========== [js-perf-test] Add a basic async-await microbenchmark These benchmarks are intended to compare the overhead of async-await vs. a naive promise implementation vs. the babel async-await transformation. The functions in the benchmark don't do any work themselves, so results should reflect only overhead of the chosen implementation. Current numbers on my local machine (higher is better): BaselineES2017-AsyncAwait(Score): 2006 BaselineNaivePromises-AsyncAwait(Score): 7470 Native-AsyncAwait(Score): 3640 BUG=v8:5639 ==========
The CQ bit was checked by jgruber@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...
On 2016/12/20 03:33:08, gsathya wrote: > On 2016/12/19 08:31:02, jgruber wrote: > > > https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... > > File test/js-perf-test/AsyncAwait/baseline-naive-promises.js (right): > > > > > https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... > > test/js-perf-test/AsyncAwait/baseline-naive-promises.js:19: return > > Promise.all(promises); > > On 2016/12/16 15:23:05, gsathya wrote: > > > promises is an array of undefineds, not an array of promises which is > > different > > > from the async await code > > > > Good catch, done. > > > > > https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... > > File test/js-perf-test/AsyncAwait/native.js (right): > > > > > https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... > > test/js-perf-test/AsyncAwait/native.js:17: for (let i = 0; i < 9; i++) await > > j(); > > On 2016/12/16 15:23:05, gsathya wrote: > > > This isn't semantically equivalent to the promise code. await j() waits > until > > > the promise j() is resolved before the next iteration starts which isn't > true > > > for Promise.all > > > > Done, removed Promise.all. > > > > > https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... > > test/js-perf-test/AsyncAwait/native.js:27: a = async function a() { return > await > > b(); }; > > On 2016/12/16 15:23:05, gsathya wrote: > > > No need for any await statements here. "return await i()" is same as "return > > > i()" in async functions > > > > Done. > > > > > https://codereview.chromium.org/2577393002/diff/20001/test/js-perf-test/Async... > > test/js-perf-test/AsyncAwait/native.js:28: > > On 2016/12/16 15:23:05, gsathya wrote: > > > i would rewrite this as > > > > > > async function a { > > > await i(); > > > await h(); > > > await h(); > > > .... > > > } > > > > The intention was to have both function nesting (a->b->...->j) and functions > > containing several async calls (i->j,j,...,j). Does that make sense? > Should the promise benchmark be h().then(() => f().then(() => ..) ? this seems > same as as single nesting, just that more closures are created. Will land this as-is for now so we can start tracking numbers, we can still adjust the benchmark as we go along.
The CQ bit was unchecked by jgruber@chromium.org
The CQ bit was checked by jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gsathya@chromium.org Link to the patchset: https://codereview.chromium.org/2577393002/#ps80001 (title: "Re-babel")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482238256173720,
"parent_rev": "e54e2dd9163b8f093967d28af667114b9948d523", "commit_rev":
"9feefafa66cdfcc3bdb90d9308d028cedfe22916"}
Message was sent while issue was closed.
Description was changed from ========== [js-perf-test] Add a basic async-await microbenchmark These benchmarks are intended to compare the overhead of async-await vs. a naive promise implementation vs. the babel async-await transformation. The functions in the benchmark don't do any work themselves, so results should reflect only overhead of the chosen implementation. Current numbers on my local machine (higher is better): BaselineES2017-AsyncAwait(Score): 2006 BaselineNaivePromises-AsyncAwait(Score): 7470 Native-AsyncAwait(Score): 3640 BUG=v8:5639 ========== to ========== [js-perf-test] Add a basic async-await microbenchmark These benchmarks are intended to compare the overhead of async-await vs. a naive promise implementation vs. the babel async-await transformation. The functions in the benchmark don't do any work themselves, so results should reflect only overhead of the chosen implementation. Current numbers on my local machine (higher is better): BaselineES2017-AsyncAwait(Score): 2006 BaselineNaivePromises-AsyncAwait(Score): 7470 Native-AsyncAwait(Score): 3640 BUG=v8:5639 Review-Url: https://codereview.chromium.org/2577393002 Cr-Commit-Position: refs/heads/master@{#41860} Committed: https://chromium.googlesource.com/v8/v8/+/9feefafa66cdfcc3bdb90d9308d028cedfe... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/v8/v8/+/9feefafa66cdfcc3bdb90d9308d028cedfe...
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2577393002/diff/80001/test/js-perf-test/JSTes... File test/js-perf-test/JSTests.json (right): https://codereview.chromium.org/2577393002/diff/80001/test/js-perf-test/JSTes... test/js-perf-test/JSTests.json:14: "main": "run.js", This currently fails on the bots. It looks like it has three sub-items that need to be specified here as well like in line 73 below. BaselineES2017 BaselineNaivePromises Native
Message was sent while issue was closed.
https://codereview.chromium.org/2577393002/diff/80001/test/js-perf-test/JSTes... File test/js-perf-test/JSTests.json (right): https://codereview.chromium.org/2577393002/diff/80001/test/js-perf-test/JSTes... test/js-perf-test/JSTests.json:14: "main": "run.js", On 2016/12/27 21:31:17, Michael Achenbach wrote: > This currently fails on the bots. It looks like it has three sub-items that need > to be specified here as well like in line 73 below. > > BaselineES2017 > BaselineNaivePromises > Native Thanks, got a CL for this at https://codereview.chromium.org/2619753002/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
