Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9956045:
Add Web Page Replay test to page cycler. (Closed)
|
Created:
8 years, 8 months ago by slamm_google Modified:
8 years, 8 months ago CC:
chromium-reviews, Aaron Boodman, pam+watch_chromium.org, mihaip+watch_chromium.org, sullivan Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd Web Page Replay test to page cycler.
- Add supporting WPR extension.
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132730
Patch Set 1 #Patch Set 2 : Ready for review. #
Total comments: 6
Patch Set 3 : Updated based on simonjam's suggestions. #Patch Set 4 : Make it easier to run replay tests manually. #Patch Set 5 : Unfork head.js, report.html. Plus, other fixes. #Patch Set 6 : Ready for review. #
Total comments: 36
Patch Set 7 : Implement review suggestions. #
Total comments: 8
Patch Set 8 : Implement review suggestions. #
Total comments: 2
Patch Set 9 : Kb -> KB #Patch Set 10 : Fix compile error. #Patch Set 11 : Fix Windows compile error. #Messages
Total messages: 22 (0 generated)
This is CL #2 of 3:
1. "webpagereplay/2012Q2/start.html" test and supporting files.
(Google internal tree)
2. performance_ui_test changes and Chrome extension to load pages:
https://chromiumcodereview.appspot.com/9956045
3. runtest.py and WPR import:
https://chromiumcodereview.appspot.com/9965045
LGTM https://chromiumcodereview.appspot.com/9956045/diff/1001/chrome/test/perf/pag... File chrome/test/perf/page_cycler_test.cc (right): https://chromiumcodereview.appspot.com/9956045/diff/1001/chrome/test/perf/pag... chrome/test/perf/page_cycler_test.cc:450: // launches replay.py to support these tests. nit: This can be reflowed. https://chromiumcodereview.appspot.com/9956045/diff/1001/chrome/test/perf/pag... chrome/test/perf/page_cycler_test.cc:581: // Web Page Replay (simulated network) tests (Windows unsupported). Why? (I know, but you should explain it to others here.) https://chromiumcodereview.appspot.com/9956045/diff/1001/tools/page_cycler/we... File tools/page_cycler/webpagereplay/extension/background.js (right): https://chromiumcodereview.appspot.com/9956045/diff/1001/tools/page_cycler/we... tools/page_cycler/webpagereplay/extension/background.js:151: this.clearAll = function(callback) { These two clear* methods should be marked private. https://chromiumcodereview.appspot.com/9956045/diff/1001/tools/page_cycler/we... tools/page_cycler/webpagereplay/extension/background.js:187: this.finishLoad = function(msg) { Should be marked private. https://chromiumcodereview.appspot.com/9956045/diff/1001/tools/page_cycler/we... tools/page_cycler/webpagereplay/extension/background.js:196: this.saveResult = function(loadTimes, timing) { private https://chromiumcodereview.appspot.com/9956045/diff/1001/tools/page_cycler/we... tools/page_cycler/webpagereplay/extension/background.js:302: resultsReadyCallback(me_); Needs _.
Thanks for the comments. I even found a bug where I had used the wrong name to clear the connections. I uploaded a new CL. -slamm On Wed, Apr 4, 2012 at 5:07 PM, <simonjam@chromium.org> wrote: > LGTM > > > https://chromiumcodereview.**appspot.com/9956045/diff/1001/** > chrome/test/perf/page_cycler_**test.cc<https://chromiumcodereview.appspot.com/9956045/diff/1001/chrome/test/perf/page_cycler_test.cc> > File chrome/test/perf/page_cycler_**test.cc (right): > > https://chromiumcodereview.**appspot.com/9956045/diff/1001/** > chrome/test/perf/page_cycler_**test.cc#newcode450<https://chromiumcodereview.appspot.com/9956045/diff/1001/chrome/test/perf/page_cycler_test.cc#newcode450> > chrome/test/perf/page_cycler_**test.cc:450: // launches replay.py to > support these tests. > nit: This can be reflowed. > > https://chromiumcodereview.**appspot.com/9956045/diff/1001/** > chrome/test/perf/page_cycler_**test.cc#newcode581<https://chromiumcodereview.appspot.com/9956045/diff/1001/chrome/test/perf/page_cycler_test.cc#newcode581> > chrome/test/perf/page_cycler_**test.cc:581: // Web Page Replay (simulated > network) tests (Windows unsupported). > Why? (I know, but you should explain it to others here.) > > https://chromiumcodereview.**appspot.com/9956045/diff/1001/** > tools/page_cycler/**webpagereplay/extension/**background.js<https://chromiumcodereview.appspot.com/9956045/diff/1001/tools/page_cycler/webpagereplay/extension/background.js> > File tools/page_cycler/**webpagereplay/extension/**background.js (right): > > https://chromiumcodereview.**appspot.com/9956045/diff/1001/** > tools/page_cycler/**webpagereplay/extension/**background.js#newcode151<https://chromiumcodereview.appspot.com/9956045/diff/1001/tools/page_cycler/webpagereplay/extension/background.js#newcode151> > tools/page_cycler/**webpagereplay/extension/**background.js:151: > this.clearAll = function(callback) { > These two clear* methods should be marked private. > > https://chromiumcodereview.**appspot.com/9956045/diff/1001/** > tools/page_cycler/**webpagereplay/extension/**background.js#newcode187<https://chromiumcodereview.appspot.com/9956045/diff/1001/tools/page_cycler/webpagereplay/extension/background.js#newcode187> > tools/page_cycler/**webpagereplay/extension/**background.js:187: > this.finishLoad = function(msg) { > Should be marked private. > > https://chromiumcodereview.**appspot.com/9956045/diff/1001/** > tools/page_cycler/**webpagereplay/extension/**background.js#newcode196<https://chromiumcodereview.appspot.com/9956045/diff/1001/tools/page_cycler/webpagereplay/extension/background.js#newcode196> > tools/page_cycler/**webpagereplay/extension/**background.js:196: > this.saveResult = function(loadTimes, timing) { > private > > https://chromiumcodereview.**appspot.com/9956045/diff/1001/** > tools/page_cycler/**webpagereplay/extension/**background.js#newcode302<https://chromiumcodereview.appspot.com/9956045/diff/1001/tools/page_cycler/webpagereplay/extension/background.js#newcode302> > tools/page_cycler/**webpagereplay/extension/**background.js:302: > resultsReadyCallback(me_); > Needs _. > > https://chromiumcodereview.**appspot.com/9956045/<https://chromiumcodereview.... >
lgtm
Please take another look. I moved everything except the archive data to the public tree. webpagereplay_util.py can be run on the command-line to start Web Page Replay and Chrome to run a test: tools/python/google/webpagereplay_utils.py 2012Q2 --auto
I am still wrapping up the fixes to page_cycler_test.cc.
I am getting a compile error on the last line of following code:
FilePath test_path = GetPageCyclerWprPath("tests");
test_path = test_path.AppendASCII(name);
test_path.ReplaceExtension(".js");
ASSERT_TRUE(file_util::DirectoryExists(test_path))
<< "Missing test directory " << test_path.value();
../../chrome/test/perf/page_cycler_test.cc: In member function 'virtual
GURL<unnamed>::PageCyclerWebPageReplayTest::GetTestUrl(const char*, bool)':
../../chrome/test/perf/page_cycler_test.cc:494: error: conversion from 'void' to
non-scalar type 'GURL' requested
Any pointers would be appreciated.
I'm guessing you can't assert from a function that returns non-void. Can you use a CHECK instead? Or move that to a void function? I dunno, I just skimmed it and I've never bothered to read how the macros are actually implemented, so this is just some guessing. James On Tue, Apr 10, 2012 at 6:33 PM, <slamm@google.com> wrote: > I am still wrapping up the fixes to page_cycler_test.cc. > > I am getting a compile error on the last line of following code: > > FilePath test_path = GetPageCyclerWprPath("tests"); > test_path = test_path.AppendASCII(name); > test_path.ReplaceExtension(".**js"); > ASSERT_TRUE(file_util::**DirectoryExists(test_path)) > << "Missing test directory " << test_path.value(); > > ../../chrome/test/perf/page_**cycler_test.cc: In member function 'virtual > GURL<unnamed>::**PageCyclerWebPageReplayTest::**GetTestUrl(const char*, > bool)': > ../../chrome/test/perf/page_**cycler_test.cc:494: error: conversion from > 'void' to > non-scalar type 'GURL' requested > > Any pointers would be appreciated. > > > https://chromiumcodereview.**appspot.com/9956045/<https://chromiumcodereview.... >
[Resending with "Reply to all".] James, that was it. I did not realize a MACRO would have a return-statement in it. I fixed it by using a parameter as the return value. I will upload a new patch shortly. I am going to see if I can eliminate or at least reduce a fork of a JavaScript file that I made. -slamm On Tue, Apr 10, 2012 at 8:19 PM, James Simonsen <simonjam@chromium.org>wrote: > I'm guessing you can't assert from a function that returns non-void. Can > you use a CHECK instead? Or move that to a void function? > > I dunno, I just skimmed it and I've never bothered to read how the macros > are actually implemented, so this is just some guessing. > > James > > > On Tue, Apr 10, 2012 at 6:33 PM, <slamm@google.com> wrote: > >> I am still wrapping up the fixes to page_cycler_test.cc. >> >> I am getting a compile error on the last line of following code: >> >> FilePath test_path = GetPageCyclerWprPath("tests"); >> test_path = test_path.AppendASCII(name); >> test_path.ReplaceExtension(".**js"); >> ASSERT_TRUE(file_util::**DirectoryExists(test_path)) >> << "Missing test directory " << test_path.value(); >> >> ../../chrome/test/perf/page_**cycler_test.cc: In member function 'virtual >> GURL<unnamed>::**PageCyclerWebPageReplayTest::**GetTestUrl(const char*, >> bool)': >> ../../chrome/test/perf/page_**cycler_test.cc:494: error: conversion from >> 'void' to >> non-scalar type 'GURL' requested >> >> Any pointers would be appreciated. >> >> >> https://chromiumcodereview.**appspot.com/9956045/<https://chromiumcodereview.... >> > >
Everyone please take another look. Almost all the changes are in this CL now. The other changes: * https://chromiumcodereview.appspot.com/9965045/ - runtest.py: Smaller diff to start WPR. - tools/build/DEPS: put WPR in tools/build/third_party (cmp, is there a better place to put WPR?) * The Google-internal CL I sent earlier now only has the replay archive file and a README. To run webpage replay and Chrome, developers can use the following commands: $CHROMIUM/src/tools/python/google/webpagereplay_utils.py 2012Q2 $CHROMIUM/src/tools/python/google/webpagereplay_utils.py 2012Q2 --record -slamm
LGTM https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... File tools/python/google/webpagereplay_utils.py (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:70: def OpenLogFile_(self): I think the _ goes on the other side in Python. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:120: 'chrome': 'src/out/{TARGET}/chrome', This is Linux only. You might want to just make it an arg to pass in the chrome binary.
Looking great. A few comments to consider. https://chromiumcodereview.appspot.com/9956045/diff/18001/chrome/test/perf/pa... File chrome/test/perf/page_cycler_test.cc (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/chrome/test/perf/pa... chrome/test/perf/page_cycler_test.cc:461: switches::kHostResolverRules, "MAP * 127.0.0.1"); Should add a TODO to remove this (with explanation). Otherwise we aren't exercising DNS at all (which is important). A kTestingFixedDnsPort would allow us to simulate DNS without root or messing with system settings. https://chromiumcodereview.appspot.com/9956045/diff/18001/chrome/test/perf/pa... chrome/test/perf/page_cycler_test.cc:470: launch_arguments_.AppendSwitch(switches::kDisableTranslate); Necessary? Seems reasonable to model it if it doesn't cause problems. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/c... File tools/page_cycler/common/report.html (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/c... tools/page_cycler/common/report.html:92: if (text.indexOf('http://localhost:8000/') == 0 || Use a regex that avoids hardcoding the port #? https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... File tools/page_cycler/webpagereplay/extension/background.js (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:12: var kWaitUrl = "http://wprwprwpr/web-page-replay-generate-200"; This extension is very closely coupled with WPR. I think it should live in that repo instead of here. What do you think? https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:49: // Map WebTiming properties to Result attribute names. Why bother with this map. Wouldn't it be more clear if we just reported the navigation timing names? https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:157: "appcache": true, Is there a way to tell browsingData to remove everything? I'm wondering how we'll remember to update this if things are added in the future. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... File tools/python/google/webpagereplay_utils.py (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:17: A file level docstring would be helpful. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:35: HTTPS_PORT = 8413 Looks like these have to match the ones hardcoded in the .cc file. Is there any way to avoid the duplicate constants? https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:150: '--testing-fixed-https-port=%s' % ReplayLauncher.HTTPS_PORT, These flags all look very similar to the ones in the .cc file. I'm confused how this works now.
Tony and James, thanks for the reviews. I have addressed most of the issues in my local-tree. I need to finish off the change James suggested to pass the chrome binary path instead of using a linux-only one. Tony, I looked at using performance_ui_tests in webpagereplay_utils.py instead of Chrome. However, using Chrome actually makes more sense, because the idea is to let developers investigate the tests. However, performance_ui_tests runs in an automated fashion. It seems like this is the price to pay for having code in both python and c++. More details in my comment below. https://chromiumcodereview.appspot.com/9956045/diff/18001/chrome/test/perf/pa... File chrome/test/perf/page_cycler_test.cc (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/chrome/test/perf/pa... chrome/test/perf/page_cycler_test.cc:461: switches::kHostResolverRules, "MAP * 127.0.0.1"); On 2012/04/13 14:32:21, tonyg wrote: > Should add a TODO to remove this (with explanation). Otherwise we aren't > exercising DNS at all (which is important). > > A kTestingFixedDnsPort would allow us to simulate DNS without root or messing > with system settings. Done. https://chromiumcodereview.appspot.com/9956045/diff/18001/chrome/test/perf/pa... chrome/test/perf/page_cycler_test.cc:470: launch_arguments_.AppendSwitch(switches::kDisableTranslate); On 2012/04/13 14:32:21, tonyg wrote: > Necessary? Seems reasonable to model it if it doesn't cause problems. When I ran the test manually, the translate requests happened at non-deterministic times. Different runs of the test would have translate requests on different page loads. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/c... File tools/page_cycler/common/report.html (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/c... tools/page_cycler/common/report.html:92: if (text.indexOf('http://localhost:8000/') == 0 || On 2012/04/13 14:32:21, tonyg wrote: > Use a regex that avoids hardcoding the port #? I just left the port off. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... File tools/page_cycler/webpagereplay/extension/background.js (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:12: var kWaitUrl = "http://wprwprwpr/web-page-replay-generate-200"; On 2012/04/13 14:32:21, tonyg wrote: > This extension is very closely coupled with WPR. I think it should live in that > repo instead of here. What do you think? The extension is what couples WPR and the page cycler, so it does not necessarily fit in either place. I decided to put it here because I was thinking of the WPR directory as server-side and the extension as client-side. The client-side js is here too. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:49: // Map WebTiming properties to Result attribute names. On 2012/04/13 14:32:21, tonyg wrote: > Why bother with this map. Wouldn't it be more clear if we just reported the > navigation timing names? I copied this from the perftracker code. A better comment would have been: // Map Performance Timing names to Result attribute names. // The Result values are the milliseconds from navigationStart. // For example, // var timing = window.performance.timing // result['total_time'] = timing.loadEventEnd - timing.navigationStart; However, how about I drop the map as you suggest and special case a computation of "total_time"? That is the only value used at the moment. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:157: "appcache": true, On 2012/04/13 14:32:21, tonyg wrote: > Is there a way to tell browsingData to remove everything? I'm wondering how > we'll remember to update this if things are added in the future. I did not see a way. I even left a few things off the list: downloads, fileSystems, formData, history, indexedDB, and passwords. One of them was giving errors, so I tried taking off items that seemed less important -- though maybe I should at least restore indexedDB. http://code.google.com/chrome/extensions/trunk/browsingData.html One way you could do it is by collecting all the chrome.browsingData.remove* method names. Or, getting the owners to add a new method to clear everything. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... File tools/python/google/webpagereplay_utils.py (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:17: On 2012/04/13 14:32:21, tonyg wrote: > A file level docstring would be helpful. Done. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:35: HTTPS_PORT = 8413 On 2012/04/13 14:32:21, tonyg wrote: > Looks like these have to match the ones hardcoded in the .cc file. Is there any > way to avoid the duplicate constants? I moved these constants to the top of the file and added a comment about the need to keep them in sync with chrome/test/perf/page_cycler_test.cc https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:70: def OpenLogFile_(self): On 2012/04/12 23:14:19, James Simonsen wrote: > I think the _ goes on the other side in Python. Done. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:120: 'chrome': 'src/out/{TARGET}/chrome', On 2012/04/12 23:14:19, James Simonsen wrote: > This is Linux only. You might want to just make it an arg to pass in the chrome > binary. Thanks for catching that. I see that runtest.py does do a fair amount of path wrangling. A nice refactor would be to move that code into platform_utils.py. However, for now I will pass the value from runtest.py and the webpagetest_utils.py command-line. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:150: '--testing-fixed-https-port=%s' % ReplayLauncher.HTTPS_PORT, On 2012/04/13 14:32:21, tonyg wrote: > These flags all look very similar to the ones in the .cc file. I'm confused how > this works now. This is for a developer wanting to investigate one of the page loads. I looked at calling performance_ui_test instead (which has a similar copy of options), however, it runs the tests automatically, has a 10 minute time limit, and closes the window as soon as the tests complete. How about I add a comment to keep these in sync with performance_ui_tests? Not ideal, but I cannot think of an easy way to have the list in one place.
Thanks for the updates. Looking forward to seeing the changes. A couple of responses inline, everything else lgtm. https://chromiumcodereview.appspot.com/9956045/diff/18001/chrome/test/perf/pa... File chrome/test/perf/page_cycler_test.cc (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/chrome/test/perf/pa... chrome/test/perf/page_cycler_test.cc:470: launch_arguments_.AppendSwitch(switches::kDisableTranslate); On 2012/04/13 23:43:36, slamm wrote: > On 2012/04/13 14:32:21, tonyg wrote: > > Necessary? Seems reasonable to model it if it doesn't cause problems. > > When I ran the test manually, the translate requests happened at > non-deterministic times. Different runs of the test would have translate > requests on different page loads. Need to check w/ Chase, but maybe that means it should be moved up to the group of flags that are shared for this test and the page cyclers. I don't see a need for it to vary between the two. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... File tools/page_cycler/webpagereplay/extension/background.js (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:12: var kWaitUrl = "http://wprwprwpr/web-page-replay-generate-200"; On 2012/04/13 23:43:36, slamm wrote: > On 2012/04/13 14:32:21, tonyg wrote: > > This extension is very closely coupled with WPR. I think it should live in > that > > repo instead of here. What do you think? > > The extension is what couples WPR and the page cycler, so it does not > necessarily fit in either place. I decided to put it here because I was thinking > of the WPR directory as server-side and the extension as client-side. The > client-side js is here too. OK, fair enough. I'm just sad to see duplication between this and the extension in WPR. Need to check w/ simonjam, but maybe we should remove the extension in WPR in favor of this one then. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:49: // Map WebTiming properties to Result attribute names. On 2012/04/13 23:43:36, slamm wrote: > On 2012/04/13 14:32:21, tonyg wrote: > > Why bother with this map. Wouldn't it be more clear if we just reported the > > navigation timing names? > > I copied this from the perftracker code. A better comment would have been: > > // Map Performance Timing names to Result attribute names. > // The Result values are the milliseconds from navigationStart. > // For example, > // var timing = window.performance.timing > // result['total_time'] = timing.loadEventEnd - timing.navigationStart; > > However, how about I drop the map as you suggest and special case a computation > of "total_time"? That is the only value used at the moment. Yeah, I like the idea of dropping the map. The perf tests can report any number of traces. I think it would be incredibly useful to report these times in addition to the total time. See here for an example of all the things that are tracked: http://build.chromium.org/f/chromium/perf/xp-release-dual-core/moz/report.htm... We could add the additional metrics to either the times tab or start a new tab with the additional metrics. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:157: "appcache": true, On 2012/04/13 23:43:36, slamm wrote: > On 2012/04/13 14:32:21, tonyg wrote: > > Is there a way to tell browsingData to remove everything? I'm wondering how > > we'll remember to update this if things are added in the future. > > I did not see a way. I even left a few things off the list: downloads, > fileSystems, formData, history, indexedDB, and passwords. One of them was giving > errors, so I tried taking off items that seemed less important -- though maybe I > should at least restore indexedDB. > > http://code.google.com/chrome/extensions/trunk/browsingData.html > > One way you could do it is by collecting all the chrome.browsingData.remove* > method names. Or, getting the owners to add a new method to clear everything. If there's no way to do them all, at least do as many as you can and leave a comment about the error on the remaining one. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... File tools/python/google/webpagereplay_utils.py (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:150: '--testing-fixed-https-port=%s' % ReplayLauncher.HTTPS_PORT, On 2012/04/13 23:43:36, slamm wrote: > On 2012/04/13 14:32:21, tonyg wrote: > > These flags all look very similar to the ones in the .cc file. I'm confused > how > > this works now. > > This is for a developer wanting to investigate one of the page loads. I looked > at calling performance_ui_test instead (which has a similar copy of options), > however, it runs the tests automatically, has a 10 minute time limit, and closes > the window as soon as the tests complete. > > How about I add a comment to keep these in sync with performance_ui_tests? Not > ideal, but I cannot think of an easy way to have the list in one place. A comment is better than nothing. But it still seems like a shame to have two harnesses as there's more duplication than just the flags. Could we instead add new flags to performance_ui_test which allow it to be run by a developer? Something that removes the timeout, suppresses closing the window, etc? If that doesn't work, then I don't see much benefit to adding it as a performance_ui_test at all. Why not just have the buildbot call the python script to run the test then? (Please correct me if I'm missing something, just thinking aloud here)
I have uploaded my latest (for the other two CLs too.) PTAL https://chromiumcodereview.appspot.com/9956045/diff/18001/chrome/test/perf/pa... File chrome/test/perf/page_cycler_test.cc (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/chrome/test/perf/pa... chrome/test/perf/page_cycler_test.cc:470: launch_arguments_.AppendSwitch(switches::kDisableTranslate); On 2012/04/16 15:34:32, tonyg wrote: > On 2012/04/13 23:43:36, slamm wrote: > > On 2012/04/13 14:32:21, tonyg wrote: > > > Necessary? Seems reasonable to model it if it doesn't cause problems. > > > > When I ran the test manually, the translate requests happened at > > non-deterministic times. Different runs of the test would have translate > > requests on different page loads. > > Need to check w/ Chase, but maybe that means it should be moved up to the group > of flags that are shared for this test and the page cyclers. I don't see a need > for it to vary between the two. I removed --disable-translate. It looks like it does one request before the test pages load. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... File tools/page_cycler/webpagereplay/extension/background.js (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:12: var kWaitUrl = "http://wprwprwpr/web-page-replay-generate-200"; On 2012/04/16 15:34:32, tonyg wrote: > On 2012/04/13 23:43:36, slamm wrote: > > On 2012/04/13 14:32:21, tonyg wrote: > > > This extension is very closely coupled with WPR. I think it should live in > > that > > > repo instead of here. What do you think? > > > > The extension is what couples WPR and the page cycler, so it does not > > necessarily fit in either place. I decided to put it here because I was > thinking > > of the WPR directory as server-side and the extension as client-side. The > > client-side js is here too. > > OK, fair enough. I'm just sad to see duplication between this and the extension > in WPR. Need to check w/ simonjam, but maybe we should remove the extension in > WPR in favor of this one then. Perhaps it could be made into a generic WPR extension if other extensions could extract the data from it. This version does work a little better than the perftracker extension now. The perftracker checked for the page being done with an interval. I found that method unreliable and it added a significant amount of time to the page loads too. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:49: // Map WebTiming properties to Result attribute names. On 2012/04/16 15:34:32, tonyg wrote: > On 2012/04/13 23:43:36, slamm wrote: > > On 2012/04/13 14:32:21, tonyg wrote: > > > Why bother with this map. Wouldn't it be more clear if we just reported the > > > navigation timing names? > > > > I copied this from the perftracker code. A better comment would have been: > > > > // Map Performance Timing names to Result attribute names. > > > // The Result values are the milliseconds from navigationStart. > > > // For example, > > > // var timing = window.performance.timing > > > // result['total_time'] = timing.loadEventEnd - timing.navigationStart; > > > > > However, how about I drop the map as you suggest and special case a > computation > > of "total_time"? That is the only value used at the moment. > > Yeah, I like the idea of dropping the map. > > The perf tests can report any number of traces. I think it would be incredibly > useful to report these times in addition to the total time. > > See here for an example of all the things that are tracked: > http://build.chromium.org/f/chromium/perf/xp-release-dual-core/moz/report.htm... > > We could add the additional metrics to either the times tab or start a new tab > with the additional metrics. The map is gone. The code is much easier to understand without it. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:157: "appcache": true, On 2012/04/16 15:34:32, tonyg wrote: > On 2012/04/13 23:43:36, slamm wrote: > > On 2012/04/13 14:32:21, tonyg wrote: > > > Is there a way to tell browsingData to remove everything? I'm wondering how > > > we'll remember to update this if things are added in the future. > > > > I did not see a way. I even left a few things off the list: downloads, > > fileSystems, formData, history, indexedDB, and passwords. One of them was > giving > > errors, so I tried taking off items that seemed less important -- though maybe > I > > should at least restore indexedDB. > > > > http://code.google.com/chrome/extensions/trunk/browsingData.html > > > > One way you could do it is by collecting all the chrome.browsingData.remove* > > method names. Or, getting the owners to add a new method to clear everything. > > If there's no way to do them all, at least do as many as you can and leave a > comment about the error on the remaining one. I added the full list again and they work fine -- I think I actually left some off because I thought they were unnecessary. I also added code to add any new remove* functions that show up. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... File tools/python/google/webpagereplay_utils.py (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:150: '--testing-fixed-https-port=%s' % ReplayLauncher.HTTPS_PORT, On 2012/04/16 15:34:32, tonyg wrote: > On 2012/04/13 23:43:36, slamm wrote: > > On 2012/04/13 14:32:21, tonyg wrote: > > > These flags all look very similar to the ones in the .cc file. I'm confused > > how > > > this works now. > > > > This is for a developer wanting to investigate one of the page loads. I looked > > at calling performance_ui_test instead (which has a similar copy of options), > > however, it runs the tests automatically, has a 10 minute time limit, and > closes > > the window as soon as the tests complete. > > > > How about I add a comment to keep these in sync with performance_ui_tests? Not > > ideal, but I cannot think of an easy way to have the list in one place. > > A comment is better than nothing. But it still seems like a shame to have two > harnesses as there's more duplication than just the flags. > > Could we instead add new flags to performance_ui_test which allow it to be run > by a developer? Something that removes the timeout, suppresses closing the > window, etc? > > If that doesn't work, then I don't see much benefit to adding it as a > performance_ui_test at all. Why not just have the buildbot call the python > script to run the test then? > > (Please correct me if I'm missing something, just thinking aloud here) I agree that there seems to be a fair amount of duplication with my current approach. I originally chose to use performance_ui_test, to help make it easier for the existing test maintainers to understand it. The duplication is certainly counter productive for maintenance. The value performance_ui_test adds that I can see: 1. Tests all listed together. 2. Adds a timeout (minor; webpagereplay_util.py would needs one) 3. Closes the browser upon completion. 4. Adds memory use and other perf stats (not sure how important they are). I would like to get what I have running on the test machines before I invest more time making it better. At this point, I do not even know if it will run and give helpful results.
https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... File tools/page_cycler/webpagereplay/extension/background.js (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:12: var kWaitUrl = "http://wprwprwpr/web-page-replay-generate-200"; On 2012/04/16 15:34:32, tonyg wrote: > On 2012/04/13 23:43:36, slamm wrote: > > On 2012/04/13 14:32:21, tonyg wrote: > > > This extension is very closely coupled with WPR. I think it should live in > > that > > > repo instead of here. What do you think? > > > > The extension is what couples WPR and the page cycler, so it does not > > necessarily fit in either place. I decided to put it here because I was > thinking > > of the WPR directory as server-side and the extension as client-side. The > > client-side js is here too. > > OK, fair enough. I'm just sad to see duplication between this and the extension > in WPR. Need to check w/ simonjam, but maybe we should remove the extension in > WPR in favor of this one then. The only thing I really liked about the old one were the explicit cold, hot, and warm test cases. I think there's 2 things that are missing: 1. Click back to get to exactly the same page. This is the hot cache case. 2. Start a new renderer, so we have no memory cache and read from the disk cache exclusively. This is the warm cache case. We get bits of #1 by navigating to another URL on the same host, which is good, but I think the other cases are very common and important to measure. We can incorporate them later though. Otherwise, I don't think anyone will run the old extension, nor do we have any reason to. WPR can focus on the server recording and replay aspect and we can get rid of the extension and appspot code. https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/python/google... File tools/python/google/webpagereplay_utils.py (right): https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/python/google... tools/python/google/webpagereplay_utils.py:216: '', '--chrome-exe', default=None, It seems this is required, but I don't see exactly where that's enforced. How will it fail if you forget it? It seems like you'll just get a bogus command line. Make sure it prints a nice message.
Last comments from me. Everything else looks good. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... File tools/page_cycler/webpagereplay/extension/background.js (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:12: var kWaitUrl = "http://wprwprwpr/web-page-replay-generate-200"; On 2012/04/17 03:30:19, James Simonsen wrote: > On 2012/04/16 15:34:32, tonyg wrote: > > On 2012/04/13 23:43:36, slamm wrote: > > > On 2012/04/13 14:32:21, tonyg wrote: > > > > This extension is very closely coupled with WPR. I think it should live in > > > that > > > > repo instead of here. What do you think? > > > > > > The extension is what couples WPR and the page cycler, so it does not > > > necessarily fit in either place. I decided to put it here because I was > > thinking > > > of the WPR directory as server-side and the extension as client-side. The > > > client-side js is here too. > > > > OK, fair enough. I'm just sad to see duplication between this and the > extension > > in WPR. Need to check w/ simonjam, but maybe we should remove the extension in > > WPR in favor of this one then. > > The only thing I really liked about the old one were the explicit cold, hot, and > warm test cases. I think there's 2 things that are missing: > > 1. Click back to get to exactly the same page. This is the hot cache case. > > 2. Start a new renderer, so we have no memory cache and read from the disk cache > exclusively. This is the warm cache case. > > We get bits of #1 by navigating to another URL on the same host, which is good, > but I think the other cases are very common and important to measure. We can > incorporate them later though. > > Otherwise, I don't think anyone will run the old extension, nor do we have any > reason to. WPR can focus on the server recording and replay aspect and we can > get rid of the extension and appspot code. Makes sense. I like the idea of porting those scenarios here and then removing the extension from the WPR codebase. I also think it would make a lot of sense to run these warm/hot test cases on the perfbots. Our behavior in that regard is completely in a blindspot right now. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... File tools/python/google/webpagereplay_utils.py (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:150: '--testing-fixed-https-port=%s' % ReplayLauncher.HTTPS_PORT, On 2012/04/16 23:43:07, slamm wrote: > On 2012/04/16 15:34:32, tonyg wrote: > > On 2012/04/13 23:43:36, slamm wrote: > > > On 2012/04/13 14:32:21, tonyg wrote: > > > > These flags all look very similar to the ones in the .cc file. I'm > confused > > > how > > > > this works now. > > > > > > This is for a developer wanting to investigate one of the page loads. I > looked > > > at calling performance_ui_test instead (which has a similar copy of > options), > > > however, it runs the tests automatically, has a 10 minute time limit, and > > closes > > > the window as soon as the tests complete. > > > > > > How about I add a comment to keep these in sync with performance_ui_tests? > Not > > > ideal, but I cannot think of an easy way to have the list in one place. > > > > A comment is better than nothing. But it still seems like a shame to have two > > harnesses as there's more duplication than just the flags. > > > > Could we instead add new flags to performance_ui_test which allow it to be run > > by a developer? Something that removes the timeout, suppresses closing the > > window, etc? > > > > If that doesn't work, then I don't see much benefit to adding it as a > > performance_ui_test at all. Why not just have the buildbot call the python > > script to run the test then? > > > > (Please correct me if I'm missing something, just thinking aloud here) > > I agree that there seems to be a fair amount of duplication with my current > approach. I originally chose to use performance_ui_test, to help make it easier > for the existing test maintainers to understand it. The duplication is certainly > counter productive for maintenance. > > The value performance_ui_test adds that I can see: > 1. Tests all listed together. > 2. Adds a timeout (minor; webpagereplay_util.py would needs one) > 3. Closes the browser upon completion. > 4. Adds memory use and other perf stats (not sure how important they are). > > I would like to get what I have running on the test machines before I invest > more time making it better. At this point, I do not even know if it will run and > give helpful results. Good point. It is important to get something running on the bots as an incremental step. But then that only requires the performance_ui_tests version, so perhaps this wrapper should be added in a subsequent patch? https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/page_cycler/w... File tools/page_cycler/webpagereplay/extension/background.js (right): https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:30: this.paint_time = 0; This is actually the time of the first paint event. Probably better named as firstPaintTime. As an aside, all these underscore variables should really be camelCase. https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:41: return Math.max(0, totalTime); Should we actually add a: CHECK(totalTime >= 0) https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/page_cycler/w... File tools/page_cycler/webpagereplay/extension/manifest.json (right): https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/manifest.json:1: { Copyright necessary?
Thank you for all the reviews. I remembered another feature of the perftracker extension. It will iterate until the variance error is under 2%. I only did not use it because the page cycler data format does not seem to allow for a variable number of iterations per page. https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... File tools/python/google/webpagereplay_utils.py (right): https://chromiumcodereview.appspot.com/9956045/diff/18001/tools/python/google... tools/python/google/webpagereplay_utils.py:150: '--testing-fixed-https-port=%s' % ReplayLauncher.HTTPS_PORT, On 2012/04/17 11:04:45, tonyg wrote: > On 2012/04/16 23:43:07, slamm wrote: > > On 2012/04/16 15:34:32, tonyg wrote: > > > On 2012/04/13 23:43:36, slamm wrote: > > > > On 2012/04/13 14:32:21, tonyg wrote: > > > > > These flags all look very similar to the ones in the .cc file. I'm > > confused > > > > how > > > > > this works now. > > > > > > > > This is for a developer wanting to investigate one of the page loads. I > > looked > > > > at calling performance_ui_test instead (which has a similar copy of > > options), > > > > however, it runs the tests automatically, has a 10 minute time limit, and > > > closes > > > > the window as soon as the tests complete. > > > > > > > > How about I add a comment to keep these in sync with performance_ui_tests? > > Not > > > > ideal, but I cannot think of an easy way to have the list in one place. > > > > > > A comment is better than nothing. But it still seems like a shame to have > two > > > harnesses as there's more duplication than just the flags. > > > > > > Could we instead add new flags to performance_ui_test which allow it to be > run > > > by a developer? Something that removes the timeout, suppresses closing the > > > window, etc? > > > > > > If that doesn't work, then I don't see much benefit to adding it as a > > > performance_ui_test at all. Why not just have the buildbot call the python > > > script to run the test then? > > > > > > (Please correct me if I'm missing something, just thinking aloud here) > > > > I agree that there seems to be a fair amount of duplication with my current > > approach. I originally chose to use performance_ui_test, to help make it > easier > > for the existing test maintainers to understand it. The duplication is > certainly > > counter productive for maintenance. > > > > The value performance_ui_test adds that I can see: > > 1. Tests all listed together. > > 2. Adds a timeout (minor; webpagereplay_util.py would needs one) > > 3. Closes the browser upon completion. > > 4. Adds memory use and other perf stats (not sure how important they are). > > > > I would like to get what I have running on the test machines before I invest > > more time making it better. At this point, I do not even know if it will run > and > > give helpful results. > > Good point. It is important to get something running on the bots as an > incremental step. But then that only requires the performance_ui_tests version, > so perhaps this wrapper should be added in a subsequent patch? runtest.py uses this module to start and stop WPR. LaunchChromium() and main() are the extra parts for running without the bots. https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/page_cycler/w... File tools/page_cycler/webpagereplay/extension/background.js (right): https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:30: this.paint_time = 0; On 2012/04/17 11:04:45, tonyg wrote: > This is actually the time of the first paint event. Probably better named as > firstPaintTime. As an aside, all these underscore variables should really be > camelCase. I am so used to staring at Python code. Thanks for the suggestion. Done. https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:41: return Math.max(0, totalTime); On 2012/04/17 11:04:45, tonyg wrote: > Should we actually add a: > CHECK(totalTime >= 0) Good suggestion. Done. https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/page_cycler/w... File tools/page_cycler/webpagereplay/extension/manifest.json (right): https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/manifest.json:1: { On 2012/04/17 11:04:45, tonyg wrote: > Copyright necessary? I thought JSON did not allow comments. Either way, Chrome seems to deal with it just fine. Done. https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/python/google... File tools/python/google/webpagereplay_utils.py (right): https://chromiumcodereview.appspot.com/9956045/diff/31001/tools/python/google... tools/python/google/webpagereplay_utils.py:216: '', '--chrome-exe', default=None, On 2012/04/17 03:30:19, James Simonsen wrote: > It seems this is required, but I don't see exactly where that's enforced. How > will it fail if you forget it? It seems like you'll just get a bogus command > line. Make sure it prints a nice message. Thanks for catching that. I made it an argument instead. The optparse documentation suggests that "required options" do not make sense. Done.
On 2012/04/17 20:47:16, slamm wrote: > The optparse documentation suggests that "required options" do not make sense. Heh. Hadn't thought about that before. :) Anyway, I took one last pass. All looks good to me. FYI, the normal process is to run the patch through trybots first and then use the commit queue to land it. I don't think you have access to either of those yet, so I'll do it for you.
Oops, meant to include this in the last message... https://chromiumcodereview.appspot.com/9956045/diff/38001/tools/page_cycler/w... File tools/page_cycler/webpagereplay/extension/background.js (right): https://chromiumcodereview.appspot.com/9956045/diff/38001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:201: result_.readBytesKb = (chrome.benchmarking.counter(kTcpReadBytes) - I'd probably go with KB here, just to make sure nobody confuses it with bits.
Thanks for moving this onto the trybots. -slamm https://chromiumcodereview.appspot.com/9956045/diff/38001/tools/page_cycler/w... File tools/page_cycler/webpagereplay/extension/background.js (right): https://chromiumcodereview.appspot.com/9956045/diff/38001/tools/page_cycler/w... tools/page_cycler/webpagereplay/extension/background.js:201: result_.readBytesKb = (chrome.benchmarking.counter(kTcpReadBytes) - On 2012/04/17 21:08:52, James Simonsen wrote: > I'd probably go with KB here, just to make sure nobody confuses it with bits. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/slamm@google.com/9956045/43016
Change committed as 132730 |
