Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19518009:
Implement 'skip_navigate_on_repeat' option (Closed)
|
Created:
7 years, 5 months ago by edmundyan Modified:
7 years, 4 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, telemetry+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement 'skip_navigate_on_repeat' option. If enabled, page runner will skip the implicit navigate action when repeating pages
BUG=263030
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217214
Patch Set 1 #
Total comments: 6
Patch Set 2 : Updating with dennis' comments #Patch Set 3 : Revised patch. Option should only be set by the test and is stored under BrowserOptions #
Total comments: 5
Patch Set 4 : Dennis changes, removing target_side_url from page attribute #Patch Set 5 : Updating with Dennis' nits #
Total comments: 2
Patch Set 6 : Adding repeat_state into RunState instance #Patch Set 7 : Rebase #
Messages
Total messages: 25 (0 generated)
So this is a little different than my last e-mail, where I suggested making an explicit 'call' action. Turns out that's more difficult than I thought, and maybe beyond the bounds of what a PageAction class is currently capable of doing. I went with a simpler approach, closer to the design doc, which only enables 'skip_navigate_on_repeat' when we are recursing into other functions.
I have some style nits. We should send this to Dave to see what he thinks about the overall approach. https://codereview.chromium.org/19518009/diff/1/tools/telemetry/telemetry/pag... File tools/telemetry/telemetry/page/page_runner_repeat.py (right): https://codereview.chromium.org/19518009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner_repeat.py:57: """Returns True if page should be navigated to""" optional nit (if you think this sounds better): 'Returns whether the given page should be navigated.' https://codereview.chromium.org/19518009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner_repeat.py:60: return False can combine the above 3 lines: return self.page_iters == 0 or not page.skip_navigate_on_repeat https://codereview.chromium.org/19518009/diff/1/tools/telemetry/telemetry/pag... File tools/telemetry/telemetry/page/page_test.py (right): https://codereview.chromium.org/19518009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_test.py:36: "skip_navigate_on_repeat", False) prefer single quotes when defining strings
Dave, can you take a look please? https://codereview.chromium.org/19518009/diff/1/tools/telemetry/telemetry/pag... File tools/telemetry/telemetry/page/page_runner_repeat.py (right): https://codereview.chromium.org/19518009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner_repeat.py:57: """Returns True if page should be navigated to""" On 2013/07/25 17:50:56, dennis_jeffrey wrote: > optional nit (if you think this sounds better): 'Returns whether the given page > should be navigated.' Done. https://codereview.chromium.org/19518009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner_repeat.py:60: return False On 2013/07/25 17:50:56, dennis_jeffrey wrote: > can combine the above 3 lines: > > return self.page_iters == 0 or not page.skip_navigate_on_repeat Done. https://codereview.chromium.org/19518009/diff/1/tools/telemetry/telemetry/pag... File tools/telemetry/telemetry/page/page_test.py (right): https://codereview.chromium.org/19518009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_test.py:36: "skip_navigate_on_repeat", False) On 2013/07/25 17:50:56, dennis_jeffrey wrote: > prefer single quotes when defining strings Done.
Sorry if we've already discussed this, but why is skip_navigate_on_repeat an attribute on the page, and not in BrowserOptions, where page_repeat and pageset_repeat are?
I was unsure what you meant when you asked this previously, and I think you missed my question asking for clarification. In anycase, I think it's a good question. Dennis, are there cases in Chrome endure where some pages in a pageset need to be reloaded every iteration while others need to have the reload skipped? In this case, having skip_navigate_on_repeat as a page attribute is the only way. Otherwise, having it just be another cmd line option like Dave mentions seems like a reasonable idea. Maybe we can bring nduca back in the loop if he has any insights since you two worked on the original spec?
On 2013/07/26 05:45:31, edmundyan wrote: > I was unsure what you meant when you asked this previously, and I think you > missed my question asking for clarification. In anycase, I think it's a good > question. > > Dennis, are there cases in Chrome endure where some pages in a pageset need to > be reloaded every iteration while others need to have the reload skipped? In > this case, having skip_navigate_on_repeat as a page attribute is the only way. > Otherwise, having it just be another cmd line option like Dave mentions seems > like a reasonable idea. Maybe we can bring nduca back in the loop if he has any > insights since you two worked on the original spec? For the Chrome Endure use case, a page needs to be only loaded the very first time. All subsequent iterations do not reload the page (because the idea is that we want the page to be opened for a very long time in order to study memory usage over time). However, in general, it may be possible for future benchmarks to want to reload some pages in a page set, but not others. That's why I thought it would be an attribute on the page. We should come to an agreement on this with Dave and Nat.
+nduca Nat, comment on whether this should be a page attribute or cmd line option?
On 2013/07/26 16:35:41, dennis_jeffrey wrote: > On 2013/07/26 05:45:31, edmundyan wrote: > > I was unsure what you meant when you asked this previously, and I think you > > missed my question asking for clarification. In anycase, I think it's a good > > question. > > > > Dennis, are there cases in Chrome endure where some pages in a pageset need to > > be reloaded every iteration while others need to have the reload skipped? In > > this case, having skip_navigate_on_repeat as a page attribute is the only way. > > > Otherwise, having it just be another cmd line option like Dave mentions seems > > like a reasonable idea. Maybe we can bring nduca back in the loop if he has > any > > insights since you two worked on the original spec? > > For the Chrome Endure use case, a page needs to be only loaded the very first > time. All subsequent iterations do not reload the page (because the idea is > that we want the page to be opened for a very long time in order to study memory > usage over time). > > However, in general, it may be possible for future benchmarks to want to reload > some pages in a page set, but not others. That's why I thought it would be an > attribute on the page. We should come to an agreement on this with Dave and > Nat. Not a command-line option. There's another class of options that can be set by a benchmark inside its test.Test class, but not available at the command line. (nduca may disagree with me on this.) Do you mean future Chrome Endure benchmarks, or theoretical future benchmarks that we don't know about yet? Any particular things in mind?
I meant arbitrary future benchmarks, not necessarily Chrome Endure-related. My feeling is that "skip_navigate_on_repeat" is something that refers to a page, so it should be possible to set that attribute on a page-by-page basis if desired. I don't see a reason yet to limit it to being set on a per-benchmark basis. On Fri, Jul 26, 2013 at 1:41 PM, <dtu@chromium.org> wrote: > On 2013/07/26 16:35:41, dennis_jeffrey wrote: > >> On 2013/07/26 05:45:31, edmundyan wrote: >> > I was unsure what you meant when you asked this previously, and I think >> you >> > missed my question asking for clarification. In anycase, I think it's a >> > good > >> > question. >> > >> > Dennis, are there cases in Chrome endure where some pages in a pageset >> need >> > to > >> > be reloaded every iteration while others need to have the reload >> skipped? >> > In > >> > this case, having skip_navigate_on_repeat as a page attribute is the >> only >> > way. > > > Otherwise, having it just be another cmd line option like Dave mentions >> > seems > >> > like a reasonable idea. Maybe we can bring nduca back in the loop if >> he has >> any >> > insights since you two worked on the original spec? >> > > For the Chrome Endure use case, a page needs to be only loaded the very >> first >> time. All subsequent iterations do not reload the page (because the idea >> is >> that we want the page to be opened for a very long time in order to study >> > memory > >> usage over time). >> > > However, in general, it may be possible for future benchmarks to want to >> > reload > >> some pages in a page set, but not others. That's why I thought it would >> be an >> attribute on the page. We should come to an agreement on this with Dave >> and >> Nat. >> > > Not a command-line option. There's another class of options that can be > set by a > benchmark inside its test.Test class, but not available at the command > line. > (nduca may disagree with me on this.) > > Do you mean future Chrome Endure benchmarks, or theoretical future > benchmarks > that we don't know about yet? Any particular things in mind? > > https://codereview.chromium.**org/19518009/<https://codereview.chromium.org/1... >
Bumping this. Could use your input Nat. :) I think the design doc and
end-state JSON should be reshaped, as it's unclear now.
The more I think about this, the more confused I get. I don't like my
implementation of defining a page attribute from a recursive call action, as it
breaks after multiple levels. Ie.
"pages": [
{
"url": "http://www.yahoo.com/",
"smoothness": { "action": "scroll" },
"endure": {
"action": "endure2",
"skip_navigate_on_repeat": true
},
"endure2": {
"action": "smoothness",
"skip_navigate_on_repeat": false
}
}
]
I suggest you two meet with dtu and come up with a plan that you three agree on. Update the design doc and let me know what you come up with?
On 2013/07/30 00:43:16, nduca wrote: > I suggest you two meet with dtu and come up with a plan that you three agree on. > Update the design doc and let me know what you come up with? So after conferring, instead of specifying 'skip_navigate_on_repeat' as a page attribute (or anywhere in the pageset JSON file), the option will be inside BrowserOptions and will be explicitly enabled when running a test from perf/benchmarks/. I've updated the doc accordingly.
Dennis/Dave could you take a look at the new patch?
https://codereview.chromium.org/19518009/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/19518009/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:126: def InitialNavigation(self, page, tab, test=None): Would naming this function "NavigatePage" make more sense here? I would think that "InitialNavigation" refers to navigating the page only the very first time, but in reading the code it looks like this function gets called any time we need to navigate the page, including when repeating a page. https://codereview.chromium.org/19518009/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner_repeat.py (right): https://codereview.chromium.org/19518009/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner_repeat.py:59: Always navigate on the first iteration of a page and on every new pageset""" add a period at the end of this line and move the closing """ down to the next line
Sorry, I should have only done 1 CL at a time. I'm incorporating some of dtu's suggestions from https://codereview.chromium.org/21174002/ because he doesn't like having target_side_url as a page attribute. Let's focus on this CL first. https://codereview.chromium.org/19518009/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/19518009/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:126: def InitialNavigation(self, page, tab, test=None): On 2013/07/31 21:09:16, dennis_jeffrey wrote: > Would naming this function "NavigatePage" make more sense here? > > I would think that "InitialNavigation" refers to navigating the page only the > very first time, but in reading the code it looks like this function gets called > any time we need to navigate the page, including when repeating a page. Yes, it will be called on every page repeat. In the future, there can be explicit 'navigate' actions inbetween an action list, which won't go through this function. I wanted to show that this is the first navigate that occurs. Maybe "ImplicitPageNavigation"? Or a docstring to explain? https://codereview.chromium.org/19518009/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner_repeat.py (right): https://codereview.chromium.org/19518009/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner_repeat.py:59: Always navigate on the first iteration of a page and on every new pageset""" On 2013/07/31 21:09:16, dennis_jeffrey wrote: > add a period at the end of this line and move the closing """ down to the next > line Done.
https://codereview.chromium.org/19518009/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/19518009/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:126: def InitialNavigation(self, page, tab, test=None): On 2013/08/01 00:14:02, edmundyan wrote: > On 2013/07/31 21:09:16, dennis_jeffrey wrote: > > Would naming this function "NavigatePage" make more sense here? > > > > I would think that "InitialNavigation" refers to navigating the page only the > > very first time, but in reading the code it looks like this function gets > called > > any time we need to navigate the page, including when repeating a page. > Yes, it will be called on every page repeat. In the future, there can be > explicit 'navigate' actions inbetween an action list, which won't go through > this function. I wanted to show that this is the first navigate that occurs. > Maybe "ImplicitPageNavigation"? Or a docstring to explain? I kinda like the "ImplicitPageNavigation" idea to differentiate this from the future code that will perform explicit page navigations. Explaining in a docstring could also help too.
On 2013/08/01 00:46:14, dennis_jeffrey wrote: > https://codereview.chromium.org/19518009/diff/21001/tools/telemetry/telemetry... > File tools/telemetry/telemetry/page/page_runner.py (right): > > https://codereview.chromium.org/19518009/diff/21001/tools/telemetry/telemetry... > tools/telemetry/telemetry/page/page_runner.py:126: def InitialNavigation(self, > page, tab, test=None): > On 2013/08/01 00:14:02, edmundyan wrote: > > On 2013/07/31 21:09:16, dennis_jeffrey wrote: > > > Would naming this function "NavigatePage" make more sense here? > > > > > > I would think that "InitialNavigation" refers to navigating the page only > the > > > very first time, but in reading the code it looks like this function gets > > called > > > any time we need to navigate the page, including when repeating a page. > > Yes, it will be called on every page repeat. In the future, there can be > > explicit 'navigate' actions inbetween an action list, which won't go through > > this function. I wanted to show that this is the first navigate that occurs. > > Maybe "ImplicitPageNavigation"? Or a docstring to explain? > > I kinda like the "ImplicitPageNavigation" idea to differentiate this from the > future code that will perform explicit page navigations. Explaining in a > docstring could also help too. Done. PTAL
LGTM; please wait to see what Dave and Nat think too. Thanks!
Dave, could you take a look please? I want to land this one before working on the explicit 'navigate_steps' change so we don't get confused.
lgtm with nit/suggestion. https://codereview.chromium.org/19518009/diff/41001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/19518009/diff/41001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:280: options.skip_navigate_on_repeat)) Hrm, I can potentially see the number of arguments ballooning over time here. Can we put repeat_state into state (the RunState instance)? I think it's also okay to pass state into _RunPage.
https://codereview.chromium.org/19518009/diff/41001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/19518009/diff/41001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:280: options.skip_navigate_on_repeat)) On 2013/08/09 23:42:04, Dave Tu wrote: > Hrm, I can potentially see the number of arguments ballooning over time here. > Can we put repeat_state into state (the RunState instance)? I think it's also > okay to pass state into _RunPage. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edmundyan@chromium.org/19518009/49001
Failed to apply patch for tools/telemetry/telemetry/core/browser_options.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file tools/telemetry/telemetry/core/browser_options.py
Hunk #1 FAILED at 50.
1 out of 1 hunk FAILED -- saving rejects to file
tools/telemetry/telemetry/core/browser_options.py.rej
Patch: tools/telemetry/telemetry/core/browser_options.py
Index: tools/telemetry/telemetry/core/browser_options.py
diff --git a/tools/telemetry/telemetry/core/browser_options.py
b/tools/telemetry/telemetry/core/browser_options.py
index
c51d7f5472b21d5155ca75808558bbc51db8349d..1892e9c78864fbbb4be74e0ac989b7e6cba58b70
100644
--- a/tools/telemetry/telemetry/core/browser_options.py
+++ b/tools/telemetry/telemetry/core/browser_options.py
@@ -50,6 +50,7 @@ class BrowserOptions(optparse.Values):
self.no_proxy_server = False
self.repeat_options = repeat_options.RepeatOptions()
+ self.skip_navigate_on_repeat = False
def Copy(self):
return copy.deepcopy(self)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edmundyan@chromium.org/19518009/53001
Message was sent while issue was closed.
Change committed as 217214 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
