Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
736653002:
[Pywebsocket PerformanceTests 2/2] Add blink_perf.pywebsocket (Closed)
Description[Pywebsocket PerformanceTests 2/2] Add blink_perf.pywebsocket
This CL adds blink_perf.pywebsocket, in which
- Pywebsocket server is started at localhost:8001, and
- Browser option --disable-web-security is added,
because we need cross-origin accesses between
memory_cache_http_server and pywebsocket server.
[1/2] PerformanceTests scripts: https://codereview.chromium.org/738753002/
[2/2] Telemetry: This CL
BUG=432408
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_perf_bisect;tryserver.chromium.perf:win_perf_bisect
Committed: https://crrev.com/ab513a3c1d3c7a14b55f26f6978ac31ffe530e00
Cr-Commit-Position: refs/heads/master@{#348149}
Patch Set 1 #Patch Set 2 : Move to PerformanceTests/Pywebsocket #Patch Set 3 : #Patch Set 4 : Rebase. #
Total comments: 2
Patch Set 5 : Remove debug code. #Patch Set 6 : Rebase. #Patch Set 7 : Move pywebsocket code outside Telemetry #
Total comments: 8
Patch Set 8 : Remove unnecessary variables #
Total comments: 2
Patch Set 9 : Rebase. Override shared_page_state. #
Total comments: 5
Patch Set 10 : Move to constructor. #
Total comments: 2
Patch Set 11 : Add StartLocalServer to platform. #Patch Set 12 : Add TODO. #
Total comments: 4
Patch Set 13 : Use shared_page_state.platform #Patch Set 14 : Rebase. #Patch Set 15 : Disable on Windows/CrOS. #Patch Set 16 : Rebase. #Patch Set 17 : Rebase. #
Messages
Total messages: 64 (20 generated)
hiroshige@chromium.org changed reviewers: + tonyg@chromium.org
Could you take a look? (FYI this depends on DEPS update https://codereview.chromium.org/738653003/ and test cases will be added to PerformanceTest/ in https://codereview.chromium.org/738753002/).
tonyg@chromium.org changed reviewers: + slamm@chromium.org
slamm@ has been doing a lot of refactoring on the way Telemetry starts/stops its servers. I think it'd be best if he reviews this in order to make sure it fits with where the code's going.
slamm@, could you take a look? Thanks.
Taking a look... On Mon, Dec 15, 2014 at 1:10 AM, <hiroshige@chromium.org> wrote: > > slamm@, could you take a look? > Thanks. > > https://codereview.chromium.org/736653002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
It seems okay to me. Chris may like to take a look. https://codereview.chromium.org/736653002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/pywebsocket_server.py (right): https://codereview.chromium.org/736653002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/pywebsocket_server.py:40: print util.GetChromiumSrcDir() Delete debug statement.
hiroshige@chromium.org changed reviewers: + chrishenry@google.com
Thanks! Chris, could you take a look? https://codereview.chromium.org/736653002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/pywebsocket_server.py (right): https://codereview.chromium.org/736653002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/pywebsocket_server.py:40: print util.GetChromiumSrcDir() On 2014/12/15 23:56:35, slamm wrote: > Delete debug statement. Done.
Is the page checked in? Can I see the page you're using? I'm wondering whether we can just run the pywebsocket server from your PageTest/Benchmark class directly instead of adding it to the core telemetry...
> Is the page checked in? Can I see the page you're using? The draft test HTML/JavaScript files are in a separate CL: https://codereview.chromium.org/738753002/ The Benchmark class is in tools/perf/benchmarks/blink_perf.py in this CL. > I'm wondering whether we can just run the pywebsocket server from your > PageTest/Benchmark class directly instead of adding it to the core telemetry... I tried this way at first, but I was not sure where to insert the server invocation code nicely in the PageTest/Benchmark class, so switched to the design like this CL. Suggestions are welcome.
chrishenry@google.com changed reviewers: + nednguyen@google.com
Hm, maybe NavigateToPage and CleanUpAfterPage? There isn't a good hook in place right now for this kind of user-specific cases, but also because we don't have enough use cases, so this is good. Adding Ned as I'll be OOO next week.
On 2014/12/20 02:34:21, chrishenry (OOO until Jan 5) wrote: > Hm, maybe NavigateToPage and CleanUpAfterPage? There isn't a good hook in place > right now for this kind of user-specific cases, but also because we don't have > enough use cases, so this is good. > > Adding Ned as I'll be OOO next week. Sorry, a bit tired. I meant: * user-specific cases of needing to perform additional environment setup that is not the standard setup provided by telemetry (can be on per-benchmark basis or per-user story run basis) * "we don't have enough use cases, so this is good" meaning it's good to see real use cases like yours instead of speculative use cases. (:
Patchset #7 (id:120001) has been deleted
Thank you for suggestion! I looked at page_test interface again and I moved pywebsocket-related to tools/perf in Patch Set 7. I used DidStartBrowser for starting and browser's _local_server_controller for shutdown to reuse my code.
https://codereview.chromium.org/736653002/diff/140001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/140001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:107: run_pywebsocket_server = True What is this variable for? https://codereview.chromium.org/736653002/diff/140001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:111: self.needs_pywebsocket_server = True also this one? https://codereview.chromium.org/736653002/diff/140001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:130: I don't see you override the ValidateAndMeasurePage or RunPage method, so what does this measurement output? https://codereview.chromium.org/736653002/diff/140001/tools/perf/benchmarks/p... File tools/perf/benchmarks/pywebsocket_server.py (right): https://codereview.chromium.org/736653002/diff/140001/tools/perf/benchmarks/p... tools/perf/benchmarks/pywebsocket_server.py:7: This file should be in telemetry/core/
https://codereview.chromium.org/736653002/diff/140001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/140001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:107: run_pywebsocket_server = True Removed. https://codereview.chromium.org/736653002/diff/140001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:111: self.needs_pywebsocket_server = True Removed. https://codereview.chromium.org/736653002/diff/140001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:130: On 2015/01/05 23:10:58, nednguyen wrote: > I don't see you override the ValidateAndMeasurePage or RunPage method, so what > does this measurement output? ValidateAndMeasurePage is overridden by the parent class, _BlinkPerfMeasurement. https://codereview.chromium.org/736653002/diff/140001/tools/perf/benchmarks/p... File tools/perf/benchmarks/pywebsocket_server.py (right): https://codereview.chromium.org/736653002/diff/140001/tools/perf/benchmarks/p... tools/perf/benchmarks/pywebsocket_server.py:7: On 2015/01/05 23:10:58, nednguyen wrote: > This file should be in telemetry/core/ I moved this to tools/perf/ according to chrishenry's comment (#10): > I'm wondering whether we can just run the pywebsocket server from your > PageTest/Benchmark class directly instead of adding it to the core telemetry... chrishenry@, how do you think?
chrishenry@, is it fine with you to have pywebsocket_server.py under telemetry/core? If so, I'll move the file. In Patch Set 7: On 2015/01/05 23:10:58, nednguyen wrote: > This file should be in telemetry/core/ On 2015/01/06 05:42:51, hiroshige wrote: > I moved this to tools/perf/ according to chrishenry's comment (#10): > > I'm wondering whether we can just run the pywebsocket server from your > > PageTest/Benchmark class directly instead of adding it to the core > telemetry...
On 2015/01/15 09:39:40, hiroshige wrote: > chrishenry@, is it fine with you to have pywebsocket_server.py under > telemetry/core? > If so, I'll move the file. > > In Patch Set 7: > On 2015/01/05 23:10:58, nednguyen wrote: > > This file should be in telemetry/core/ > On 2015/01/06 05:42:51, hiroshige wrote: > > I moved this to tools/perf/ according to chrishenry's comment (#10): > > > I'm wondering whether we can just run the pywebsocket server from your > > > PageTest/Benchmark class directly instead of adding it to the core > > telemetry... Ned, the reason why I asked for the switch is because I don't want to add extra maintenance burden for core telemetry code for this one use case. Once we have more, we can move and generalize. Thoughts?
nednguyen, could you take a look at chrishenry's comment in #20? Thanks.
nednguyen@google.com changed reviewers: + sullivan@chromium.org - tonyg@chromium.org
ok, I think I was confused when seeing client code deal with these server setup logic through browser object rather than platform. https://codereview.chromium.org/736653002/diff/160001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/160001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:120: def DidStartBrowser(self, browser): Don't rely on the DidStartBrowser hook, you will want to override shared_page_state to setup your custom local server instead.
Patchset #9 (id:180001) has been deleted
https://codereview.chromium.org/736653002/diff/160001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/160001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:120: def DidStartBrowser(self, browser): On 2015/01/21 16:45:48, nednguyen wrote: > Don't rely on the DidStartBrowser hook, you will want to override > shared_page_state to setup your custom local server instead. Done.
nednguyen@google.com changed reviewers: + aiolos@chromium.org - chrishenry@google.com, slamm@chromium.org
I think this patch is landable. +Kari, can you also take a pass at this? https://codereview.chromium.org/736653002/diff/200001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/200001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:138: pywebsocket_server.PywebsocketServer, None): Can you explain why we need this check?
https://codereview.chromium.org/736653002/diff/200001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/200001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:138: pywebsocket_server.PywebsocketServer, None): On 2015/04/06 17:34:02, nednguyen wrote: > Can you explain why we need this check? This is to avoid multiple instances of pywebsocket servers to be started. WillRunUserStory is called for every html page under PerformanceTests/Pywebsocket, but pywebsocket should be started only once. Similar logic is implemented in SetHTTPServerDirectories for HTTP server: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
https://codereview.chromium.org/736653002/diff/200001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/200001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:138: pywebsocket_server.PywebsocketServer, None): On 2015/04/08 07:47:25, hiroshige wrote: > On 2015/04/06 17:34:02, nednguyen wrote: > > Can you explain why we need this check? > This is to avoid multiple instances of pywebsocket servers to be started. > WillRunUserStory is called for every html page under > PerformanceTests/Pywebsocket, but pywebsocket should be started only once. > Similar logic is implemented in SetHTTPServerDirectories for HTTP server: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... Ah, I see. Then this functionality probably should go to the __init__ of _SharedPywebsocketPageState. See https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... You also want to add some clean up logic in TearDownState?
https://codereview.chromium.org/736653002/diff/200001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/200001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:138: pywebsocket_server.PywebsocketServer, None): On 2015/04/08 15:08:26, nednguyen wrote: > On 2015/04/08 07:47:25, hiroshige wrote: > > On 2015/04/06 17:34:02, nednguyen wrote: > > > Can you explain why we need this check? > > This is to avoid multiple instances of pywebsocket servers to be started. > > WillRunUserStory is called for every html page under > > PerformanceTests/Pywebsocket, but pywebsocket should be started only once. > > Similar logic is implemented in SetHTTPServerDirectories for HTTP server: > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > Ah, I see. > Then this functionality probably should go to the __init__ of > _SharedPywebsocketPageState. > See > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > You also want to add some clean up logic in TearDownState? +1. You should only have logic that should be run prior to every user story/page in WillRunUserStory.
https://codereview.chromium.org/736653002/diff/200001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/200001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:138: pywebsocket_server.PywebsocketServer, None): On 2015/04/08 17:32:56, aiolos wrote: > On 2015/04/08 15:08:26, nednguyen wrote: > > On 2015/04/08 07:47:25, hiroshige wrote: > > > On 2015/04/06 17:34:02, nednguyen wrote: > > > > Can you explain why we need this check? > > > This is to avoid multiple instances of pywebsocket servers to be started. > > > WillRunUserStory is called for every html page under > > > PerformanceTests/Pywebsocket, but pywebsocket should be started only once. > > > Similar logic is implemented in SetHTTPServerDirectories for HTTP server: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > Ah, I see. > > Then this functionality probably should go to the __init__ of > > _SharedPywebsocketPageState. > > See > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > You also want to add some clean up logic in TearDownState? > > +1. You should only have logic that should be run prior to every user story/page > in WillRunUserStory. Done (I'm not sure this is correct way to get platform_backend though).
Hi Hiroshi, I think this is definitely in the right direction, but the API isn't quite right yet. This is because the server_controller stuff is supposed to be owned by platform, but slamm@ hasn't been able to finish it yet (https://code.google.com/p/chromium/issues/detail?id=404771) He already moved to another team & work on a different project, do you want to finish the move of server_controller to platform? https://codereview.chromium.org/736653002/diff/220001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/220001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:139: backend = platform._platform_backend # pylint: disable=W0212 No don't do this, platform_backend is not a public API.
On 2015/04/09 15:41:51, nednguyen wrote: > Hi Hiroshi, I think this is definitely in the right direction, but the API isn't > quite right yet. > > This is because the server_controller stuff is supposed to be owned by platform, > but slamm@ hasn't been able to finish it yet > (https://code.google.com/p/chromium/issues/detail?id=404771) > > He already moved to another team & work on a different project, do you want to > finish the move of server_controller to platform? Hmm. I took a look at the crbug and related code, but it seems to take some time because it will be a medium-sized refactoring of telemetry (and also I'm not familiar with the features that would be affected by the change of API). How about landing Patch Set 9? Or is there any way to work around the Issue 404771?
On 2015/04/22 10:39:29, hiroshige wrote: > On 2015/04/09 15:41:51, nednguyen wrote: > > Hi Hiroshi, I think this is definitely in the right direction, but the API > isn't > > quite right yet. > > > > This is because the server_controller stuff is supposed to be owned by > platform, > > but slamm@ hasn't been able to finish it yet > > (https://code.google.com/p/chromium/issues/detail?id=404771) > > > > He already moved to another team & work on a different project, do you want to > > finish the move of server_controller to platform? > Hmm. > I took a look at the crbug and related code, but it seems to take some time > because it will be a medium-sized refactoring of telemetry (and also I'm not > familiar with the features that would be affected by the change of API). > How about landing Patch Set 9? Or is there any way to work around the Issue > 404771? I think the shortest work around is probably copying related local_server_controller API in browser.py to platform/__init__.py that enable you to land this. Then you can leave a TODO in browser.py to remove those APIs in browser and telemetry team will do the rest of work of moving existing code to platform.StartLocalServer
Patchset #12 (id:260001) has been deleted
> I think the shortest work around is probably copying related > local_server_controller API in browser.py to platform/__init__.py that enable > you to land this. Then you can leave a TODO in browser.py to remove those APIs > in browser and telemetry team will do the rest of work of moving existing code > to platform.StartLocalServer Thanks for suggestions! How about Patch Set 12? I manually call Close() of the local server in SharedPageState.TearDownState().
https://codereview.chromium.org/736653002/diff/220001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/220001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:139: backend = platform._platform_backend # pylint: disable=W0212 On 2015/04/09 15:41:51, nednguyen wrote: > No don't do this, platform_backend is not a public API. Done.
https://codereview.chromium.org/736653002/diff/280001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/280001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:139: platform_module.GetHostPlatform().StartLocalServer( Does this benchmark work for the case browser runs on remote platform like android & cros?
https://codereview.chromium.org/736653002/diff/280001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/280001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:139: platform_module.GetHostPlatform().StartLocalServer( On 2015/04/23 15:29:19, nednguyen wrote: > Does this benchmark work for the case browser runs on remote platform like > android & cros? I tested and, no, this didn't work on android trybots... (Patch Set 9 worked)
https://codereview.chromium.org/736653002/diff/280001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/280001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:139: platform_module.GetHostPlatform().StartLocalServer( On 2015/04/24 08:15:44, hiroshige wrote: > On 2015/04/23 15:29:19, nednguyen wrote: > > Does this benchmark work for the case browser runs on remote platform like > > android & cros? > > I tested and, no, this didn't work on android trybots... > (Patch Set 9 worked) Then we should figure out patch 12 doesn't work on Android but patchset 9 worked. My suspicion is there are some extra steps to forward the host port to the remote port that isn't wired up properly in this limbo state. Once we figure that out, you can decide whether to: 1) File a bug about it, and disable this benchmark on Android & Cros with @benchmark.Disabled('android', 'chromeos') # crbug.com/... 2) Fix the problem in this patch.
Patchset #13 (id:300001) has been deleted
Patchset #14 (id:340001) has been deleted
Patchset #14 (id:360001) has been deleted
Sorry for delay. I fixed the problem on Android in Patch Set 13, and rebased in Patch Set 14. Now this CL works on: Linux, Mac, Android (Not sure on CrOS because there is no CrOS trybot) But it doesn't work on Windows (works well locally, timeout on trybots). How about landing Patch Set 14 + disabling the test on Windows? If this is fine, then I'll create a crbug entry and update the CL. https://codereview.chromium.org/736653002/diff/280001/tools/perf/benchmarks/b... File tools/perf/benchmarks/blink_perf.py (right): https://codereview.chromium.org/736653002/diff/280001/tools/perf/benchmarks/b... tools/perf/benchmarks/blink_perf.py:139: platform_module.GetHostPlatform().StartLocalServer( This was because on Android platform.GetPlatformForDevice(device, finder_options) should be used instead of platform_module.GetHostPlatform() on Desktop. In Patch Set 13, I use SharedPageState.platform (which obtains platform via browser) and this works on Android.
FYI Test results on Windows trybots: https://codereview.chromium.org/1268523005/#ps1 They timed out after 1-hour time limit. The test seems to hang after ServeForever() in pywebsocket_server.py is executed and before any test is executed.
nednguyen@google.com changed reviewers: + cylee@google.com
lgtm I think you want to disable the test on both Windows & cros. Also please take time to review our new policy about adding benchmark here: https://docs.google.com/document/d/1bBKyYCW3VlUUPDpQE4xvrMFdA6tovQMZoqO9KCcmq...
Thanks for reviewing! I disabled Windows&cros in Patch Set 15. I'll commit this after depending CLs are landed.
Patchset #19 (id:480001) has been deleted
Patchset #18 (id:460001) has been deleted
Patchset #17 (id:440001) has been deleted
The CQ bit was checked by hiroshige@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/736653002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/736653002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_nexus5_perf_bisect on tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nednguyen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/736653002/#ps500001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736653002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/736653002/500001
On 2015/09/10 10:35:34, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > android_nexus5_perf_bisect on tryserver.chromium.perf (JOB_TIMED_OUT, no build > URL) Removed Android Nexus 5 from extra cq due to job timed out.
Message was sent while issue was closed.
Committed patchset #17 (id:500001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/ab513a3c1d3c7a14b55f26f6978ac31ffe530e00 Cr-Commit-Position: refs/heads/master@{#348149}
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:500001) has been created in https://codereview.chromium.org/1333153002/ by rbyers@chromium.org. The reason for reverting is: New test on the reference build causing Linux and Mac perf bots to fail. http://crbug.com/530374.
Message was sent while issue was closed.
Created a relanding CL: https://codereview.chromium.org/1331403002/
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/ab513a3c1d3c7a14b55f26f6978ac31ffe530e00 Cr-Commit-Position: refs/heads/master@{#348149} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
