Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2482793002:
wptserve: Enable WPTServe by default. (Closed)
Descriptionwptserve: Enable WPTServe by default.
Feel free to revert this if you see stability issues of imported/wpt tests in
webkit_tests step.
BUG=618366
Committed: https://crrev.com/311d6ccdab758c38b0394a06d054a4db15a5fa21
Cr-Commit-Position: refs/heads/master@{#431186}
Patch Set 1 #
Messages
Total messages: 30 (12 generated)
The CQ bit was checked by tkent@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 ========== wptserve: Enable WPTServe by default. Feel free to revert this if you see stability issues of imported/wpt tests in webit_tests step. BUG=618366 ========== to ========== wptserve: Enable WPTServe by default. Feel free to revert this if you see stability issues of imported/wpt tests in webkit_tests step. BUG=618366 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
tkent@chromium.org changed reviewers: + jsbell@chromium.org, qyearsley@chromium.org
jsbell@, qyearsley@, please review this! I think all tests in WPTServeExpectation work correctly.
\o/ lgtm
On 2016/11/07 at 17:32:53, jsbell wrote: > \o/ lgtm On 2016/11/07 at 17:32:53, jsbell wrote: > \o/ lgtm This is exciting, although note that this will increase time taken for a layout tests run by about 5 minutes, based on runs of WebKit Linux Precise and WebKit Linux - WPTServe: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis... https://build.chromium.org/p/chromium.fyi/builders/WebKit%20Linux%20-%20WPTSe... Although this may not matter as soon as layout tests can be run with swarming, so I think switching to use wptserve by default for all wpt tests LGTM; rbyers@, do you know of anyone else that would want to OK this change before committing?
On 2016/11/07 19:51:36, qyearsley wrote: > On 2016/11/07 at 17:32:53, jsbell wrote: > > \o/ lgtm > > On 2016/11/07 at 17:32:53, jsbell wrote: > > \o/ lgtm > > This is exciting, although note that this will increase time taken for a layout > tests run by about 5 minutes, based on runs of WebKit Linux Precise and WebKit > Linux - WPTServe: > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis... > https://build.chromium.org/p/chromium.fyi/builders/WebKit%20Linux%20-%20WPTSe... Ouch, that does not sound good to me. Why is it that much slower? It looks like there's only ~2250 tests. Out of ~40k, making things 30% slower (21 minutes instead of 16) is a very big regression. We shouldn't land this unless we understand where that time is going and what can be done to speed it up. It looks like imported/wpt/service-workers/service-work is substantially slower (26 seconds compared to 228 w/ wptserve), and some of the other directories look somewhat slower, but even with that that doesn't explain why things would be 5 minutes of wall clock time (40 minutes of CPU time across 8 workers) slower.
On 2016/11/07 at 20:19:47, dpranke wrote: > On 2016/11/07 19:51:36, qyearsley wrote: > > On 2016/11/07 at 17:32:53, jsbell wrote: > > > \o/ lgtm > > > > On 2016/11/07 at 17:32:53, jsbell wrote: > > > \o/ lgtm > > > > This is exciting, although note that this will increase time taken for a layout > > tests run by about 5 minutes, based on runs of WebKit Linux Precise and WebKit > > Linux - WPTServe: > > > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis... > > https://build.chromium.org/p/chromium.fyi/builders/WebKit%20Linux%20-%20WPTSe... > > Ouch, that does not sound good to me. > > Why is it that much slower? It looks like there's only ~2250 tests. Out of ~40k, making things 30% > slower (21 minutes instead of 16) is a very big regression. We shouldn't land this unless we understand > where that time is going and what can be done to speed it up. It seems this is a Linux-only issue. Mac and Windows don't have such significant difference. I'll investigate. Vanilla Mac: 17:02 WPTServe Mac: 18.36 Vanilla Win: 22:03 WPTServe Win: 22:30 Vanilla Linux: 16:10 WPTServe Linux: 21:42 https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.11/build... https://build.chromium.org/p/chromium.fyi/builders/WebKit%20Mac%20-%20WPTServ... https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/47611 https://build.chromium.org/p/chromium.fyi/builders/WebKit%20Win%20-%20WPTServ... https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis... https://build.chromium.org/p/chromium.fyi/builders/WebKit%20Linux%20-%20WPTSe... > It looks like imported/wpt/service-workers/service-work is substantially slower (26 seconds compared > to 228 w/ wptserve), and some of the other directories look somewhat slower, but even with > that that doesn't explain why things would be 5 minutes of wall clock time (40 minutes > of CPU time across 8 workers) slower. This service-worker behavior is expected, I think. Without wptserve, all of service-worker tests fail immediatedy because service worker doesn't work with file: URLs, and with wptserve some tests do some meaningful jobs, and some tests time out.
This is a tangent but... > > It looks like imported/wpt/service-workers/service-work is substantially > slower (26 seconds compared > > to 228 w/ wptserve), and some of the other directories look somewhat slower, > but even with > > that that doesn't explain why things would be 5 minutes of wall clock time (40 > minutes > > of CPU time across 8 workers) slower. > > This service-worker behavior is expected, I think. Without wptserve, all of > service-worker tests fail immediatedy because service worker doesn't work with > file: URLs, and with wptserve some tests do some meaningful jobs, and some tests > time out. How long does http/tests/serviceworker take? WPT's imported/wpt/service-workers is derived from it; I would therefore naively expect the times to be broadly similar if it's just what the tests are doing. After some inspection we should be able to dedupe our local copies. If that makes up the time, great! If not, something else is going on.
On 2016/11/08 at 08:15:40, tkent wrote: > On 2016/11/07 at 20:19:47, dpranke wrote: > > On 2016/11/07 19:51:36, qyearsley wrote: > > > On 2016/11/07 at 17:32:53, jsbell wrote: > > > > \o/ lgtm > > > > > > On 2016/11/07 at 17:32:53, jsbell wrote: > > > > \o/ lgtm > > > > > > This is exciting, although note that this will increase time taken for a layout > > > tests run by about 5 minutes, based on runs of WebKit Linux Precise and WebKit > > > Linux - WPTServe: > > > > > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis... > > > https://build.chromium.org/p/chromium.fyi/builders/WebKit%20Linux%20-%20WPTSe... > > > > Ouch, that does not sound good to me. > > > > Why is it that much slower? It looks like there's only ~2250 tests. Out of ~40k, making things 30% > > slower (21 minutes instead of 16) is a very big regression. We shouldn't land this unless we understand > > where that time is going and what can be done to speed it up. > > It seems this is a Linux-only issue. Mac and Windows don't have such significant difference. I'll investigate. > > Vanilla Mac: 17:02 > WPTServe Mac: 18.36 > > Vanilla Win: 22:03 > WPTServe Win: 22:30 > > Vanilla Linux: 16:10 > WPTServe Linux: 21:42 I didn't see significant difference on my local Linux box (16 hardware threads, 16GB RAM) Vanilla: 22:34 WPTServe: 22:48 > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis... > https://build.chromium.org/p/chromium.fyi/builders/WebKit%20Linux%20-%20WPTSe... I found these two slaves were not same hardware. vm2-m1 for "WebKit Linux Precise": https://build.chromium.org/p/chromium.webkit/buildslaves/vm2-m1 memory total: 15.67 GB os family: Debian os version: 12.04 processor count: 8 processor type: Intel(R) Xeon(R) CPU E5-2670 0 @ 2.60GHz product name: VMware Virtual Platform slave72-c1 for "WebKit Linux - WPServe": https://build.chromium.org/p/chromium.fyi/buildslaves/slave72-c1 memory total: 29.45 GB os family: Debian os version: 12.04 processor count: 8 processor type: Intel(R) Xeon(R) CPU @ 2.30GHz product name: Google Compute Engine The former has faster CPUs, and difference of VMware and GCP might affect. So, IMO we should try this CL. Any objection?
On 2016/11/09 at 00:55:49, tkent wrote: > On 2016/11/08 at 08:15:40, tkent wrote: > > On 2016/11/07 at 20:19:47, dpranke wrote: > > > On 2016/11/07 19:51:36, qyearsley wrote: > > > > On 2016/11/07 at 17:32:53, jsbell wrote: > > > > > \o/ lgtm > > > > > > > > On 2016/11/07 at 17:32:53, jsbell wrote: > > > > > \o/ lgtm > > > > > > > > This is exciting, although note that this will increase time taken for a layout > > > > tests run by about 5 minutes, based on runs of WebKit Linux Precise and WebKit > > > > Linux - WPTServe: > > > > > > > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis... > > > > https://build.chromium.org/p/chromium.fyi/builders/WebKit%20Linux%20-%20WPTSe... > > > > > > Ouch, that does not sound good to me. > > > > > > Why is it that much slower? It looks like there's only ~2250 tests. Out of ~40k, making things 30% > > > slower (21 minutes instead of 16) is a very big regression. We shouldn't land this unless we understand > > > where that time is going and what can be done to speed it up. > > > > It seems this is a Linux-only issue. Mac and Windows don't have such significant difference. I'll investigate. > > > > Vanilla Mac: 17:02 > > WPTServe Mac: 18.36 > > > > Vanilla Win: 22:03 > > WPTServe Win: 22:30 > > > > Vanilla Linux: 16:10 > > WPTServe Linux: 21:42 > > I didn't see significant difference on my local Linux box (16 hardware threads, 16GB RAM) > > Vanilla: 22:34 > WPTServe: 22:48 > > > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis... > > https://build.chromium.org/p/chromium.fyi/builders/WebKit%20Linux%20-%20WPTSe... > > I found these two slaves were not same hardware. > > vm2-m1 for "WebKit Linux Precise": https://build.chromium.org/p/chromium.webkit/buildslaves/vm2-m1 > memory total: 15.67 GB > os family: Debian > os version: 12.04 > processor count: 8 > processor type: Intel(R) Xeon(R) CPU E5-2670 0 @ 2.60GHz > product name: VMware Virtual Platform > > slave72-c1 for "WebKit Linux - WPServe": https://build.chromium.org/p/chromium.fyi/buildslaves/slave72-c1 > memory total: 29.45 GB > os family: Debian > os version: 12.04 > processor count: 8 > processor type: Intel(R) Xeon(R) CPU @ 2.30GHz > product name: Google Compute Engine > > The former has faster CPUs, and difference of VMware and GCP might affect. > So, IMO we should try this CL. Any objection? Ah, good news, thanks for looking into it :-) So the expected change in run time is probably more like what it is for Mac and Win - more like a 1 minute increase, or 2-5% increase, probably?
The CQ bit was checked by tkent@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/11/09 at 16:22:54, qyearsley wrote: > So the expected change in run time is probably more like what it is for Mac and Win - more like a 1 minute increase, or 2-5% increase, probably? I guess so. Let's enable WTPServe by default, check time difference, and revert if we see unexpected result.
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 tkent@chromium.org
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.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== wptserve: Enable WPTServe by default. Feel free to revert this if you see stability issues of imported/wpt tests in webkit_tests step. BUG=618366 ========== to ========== wptserve: Enable WPTServe by default. Feel free to revert this if you see stability issues of imported/wpt tests in webkit_tests step. BUG=618366 Committed: https://crrev.com/311d6ccdab758c38b0394a06d054a4db15a5fa21 Cr-Commit-Position: refs/heads/master@{#431186} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/311d6ccdab758c38b0394a06d054a4db15a5fa21 Cr-Commit-Position: refs/heads/master@{#431186}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2495613002/ by dpranke@chromium.org. The reason for reverting is: I'm reverting this because I think imported/wpt/html/browsers/history/the-location-interface/reload_post_1.html started failing with the switch on windows, see https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%... and I'm the sheriff today. However, I'm also concerned that I think you missed my comments in #10. I don't think a 30% cycle time regression is okay, at least not without an explanation and a plan for addressing it..
Message was sent while issue was closed.
On 2016/11/10 at 17:53:42, dpranke wrote: > A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2495613002/ by dpranke@chromium.org. > > The reason for reverting is: > I'm reverting this because I think > > imported/wpt/html/browsers/history/the-location-interface/reload_post_1.html > > started failing with the switch on windows, see > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%... > > and I'm the sheriff today. > > However, I'm also concerned that I think you missed my comments in #10. I don't think a 30% cycle time regression is okay, > at least not without an explanation and a plan for addressing it.. Note, it appeared that my initial estimate of ~30% increase in run time based on comparing "WebKit Linux - WPTServe" and "WebKit Linux Precise" was incorrect -- that difference could be at least partially explained by the fact that the latter had faster hardware. The real difference is probably closer to 1% - in https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis... it took about 17.5 minutes, versus about 16 minutes without the flag earlier (looking at build 2109).
Message was sent while issue was closed.
On 2016/11/10 17:53:42, Dirk Pranke wrote: > However, I'm also concerned that I think you missed my comments in #10. I don't > think a 30% cycle time regression is okay, > at least not without an explanation and a plan for addressing it.. Explanation was in #13 - no local repro for the time difference, and the bots had different configurations. Plan was to try and see if there was a real time regression. +1 to revert until you're confident, of course. Hopefully we got enough runs through before the revert to analyze. ... Hrm... the test changed from timeout detected by run-webkit-tests ([Timeout] expectation) to timeout detected by testharness.js (Text output containing TIMEOUT). That seems like it would have been a lurking flakiness issue anyway as the two timers race. I don't know how we've handled those in the past. As an aside, I repro the timeout w/ that test in Chrome 54: http://w3c-test.org/html/browsers/history/the-location-interface/reload_post_... ... so we should fix the test or Chrome. :P
Message was sent while issue was closed.
On 2016/11/10 at 18:13:22, jsbell wrote: > > Hrm... the test changed from timeout detected by run-webkit-tests ([Timeout] expectation) to timeout detected by testharness.js (Text output containing TIMEOUT). That seems like it would have been a lurking flakiness issue anyway as the two timers race. I don't know how we've handled those in the past. This has been a problem for newly-imported w3c tests several times -- filed bug http://crbug.com/664268 for this "text TIMEOUT vs test runner TIMEOUT" issue.
Message was sent while issue was closed.
On 2016/11/10 at 17:53:42, dpranke wrote: > and I'm the sheriff today. > > However, I'm also concerned that I think you missed my comments in #10. I don't think a 30% cycle time regression is okay, > at least not without an explanation and a plan for addressing it.. I'm sorry for adding extra sheriff jobs. I already explained about the 30% cycle time regression, and now we verified it was a false alarm. Time increase by this CL was less than 30 seconds on the Linux bot. I'll show more data later.
Message was sent while issue was closed.
The following data shows webkit_tests time of five runs before/after enabling WTPServe. Roughly speaking, +1 minute in release build, +3 minutes in debug build. I think they are acceptable. WebKit Win7 BEFORE: 22:58 22:41 23:12 22:44 23:03 median=22:58 avg=22:55 AFTER: 25:09 24:48 23:27 24:52 24:49 median=24:49 avg=24:37 WebKit Win7 (dbg) BEFORE: 1:42:54 1:43:20 1:45:29 1:43:58 1:42:38 median=1:43:20 avg=1:43:40 AFTER: 1:49:43 1:47:31 1:46:21 1:40:52 1:43:18 median=1:46:21 avg=1:45:33 WebKit Mac10.11 BEFORE: 18:25 18:43 18:15 17:46 17:47 median=18:15 avg=18:11 AFTER: 17:45 18:56 19:40 19:21 19:22 median=18:56 avg=19:00 WebKit Mac 10.11 (dbg) BEFORE: 1:04:34 1:04:34 1:04:02 1:02:44 1:04:49 median=1:04:34 avg=1:04:09 AFTER : 1:02:23 1:06:40 1:04:52 1:07:07 1:04:46 median=1:04:52 avg=1:05:10 WebKit Linux Precise BEFORE: 17:13 16:53 17:03 16:54 17:14 median=17:03 avg=17:03 AFTER: 17:54 17:36 17:51 17:43 18:28 median=17:51 avg=17:54 WebKit Linux Precise (dbg) BEFORE: 1:26:47 1:27:29 1:25:37 1:26:46 1:27:03 median=1:26:47 avg=1:26:44 AFTER: 1:29:07 1:30:58 1:27:03 1:30:07 1:29:41 median=1:29:41 avg=1:29:23 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
