Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
907503002:
telemetry: Create a helper class to quickly extend profiles. (Closed)
Descriptiontelemetry: Create a helper class to quickly extend profiles.
I added the class FastNavigationProfileExtender, which is not yet hooked up to
any profile creators. The class batch navigates tabs in a browser, which allows
a profile creator to quickly perform a large number of URL navigations.
BUG=442546
Committed: https://crrev.com/16b5175f8967c8ea8430f7ada22eeb8586776f9a
Cr-Commit-Position: refs/heads/master@{#316738}
Patch Set 1 #Patch Set 2 : Minor fixes. #
Total comments: 4
Patch Set 3 : Comments from nednguyen. #
Total comments: 18
Patch Set 4 : #
Total comments: 5
Patch Set 5 : Comments from nednguyen, round three. #Patch Set 6 : Modify profile_generator to allow more types of classes in the profile_generator/ directory. #
Total comments: 27
Patch Set 7 : Comments from nednguyen. #Patch Set 8 : Rename _PAGE_LOAD_TIMEOUT_IN_SECONDS. #Patch Set 9 : Remove changes to profile_generator.py. #
Total comments: 2
Patch Set 10 : Wait for tab urls to change as a sign that navigation has started. #
Total comments: 4
Messages
Total messages: 50 (13 generated)
erikchen@chromium.org changed reviewers: + nednguyen@google.com
nednguyen: Please review. I've intentionally avoided using PageTest, SharedPageState, UserStory, UserStoryRunner, etc. because the existing behavior of those classes does not match their expected behavior after your refactor, and there does not exist a precise refactor plan. Once the classes have been refactored, it should be easier to make some minor modifications to this class to insert it back into the standard class hierarchy.
On 2015/02/06 19:51:27, erikchen wrote: > nednguyen: Please review. > > I've intentionally avoided using PageTest, SharedPageState, UserStory, > UserStoryRunner, etc. because the existing behavior of those classes does not > match their expected behavior after your refactor, and there does not exist a > precise refactor plan. Once the classes have been refactored, FYI, it will be a multi-quarters effort to refactor all of them to the right place. Whereas SharedUserStoryState & UserStory are fairly stable APIs. > it should be > easier to make some minor modifications to this class to insert it back into the > standard class hierarchy.
I think your approach of creating a base class for profile extender here is good. However, I think we may not need user_story/user_story_runner/benchmark/... for what you need. https://codereview.chromium.org/907503002/diff/20001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/20001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:35: profile_extender.RunUserStory() You will only need these two hooks if you think they are extension points for other profiler to implement the behavior differently. https://codereview.chromium.org/907503002/diff/20001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:39: profile_extender.TearDownState() Also this one.
nednguyen: PTAL https://codereview.chromium.org/907503002/diff/20001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/20001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:35: profile_extender.RunUserStory() On 2015/02/09 20:57:18, nednguyen wrote: > You will only need these two hooks if you think they are extension points for > other profiler to implement the behavior differently. Renamed the hooks to BrowserSetup() and PerformNavigations() https://codereview.chromium.org/907503002/diff/20001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:39: profile_extender.TearDownState() On 2015/02/09 20:57:18, nednguyen wrote: > Also this one. Renamed to BrowserTeardown()
Please provide unittest coverage for this. I think that would change how you want to design fast_navigation_profile_extender's methods a little bit. https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:31: self._navigation_urls = self.NavigationUrls() def GetUrlsToNavigate() https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:50: self._queued_tabs = [] What about making this a map from tabs to intial_urls? https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:53: self._navigation_url_index = 0 I don't like having this counter as a class's member. The more side-effect-free methods, the easier it's to understand & unittest your implementation, and it's less error prone. https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:62: self._BrowserSetup(finder_options) s/_BrowserSetup/_SetupBrowser https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:64: except: you don't need the "except:\nraise:" https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:67: self._BrowserTeardown() s/_BrowserTeardown/_TeardownBrowser()
Patchset #4 (id:60001) has been deleted
On 2015/02/09 21:35:03, nednguyen wrote: > Please provide unittest coverage for this. I think that would change how you > want to design fast_navigation_profile_extender's methods a little bit. > > https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... > File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): > > https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... > tools/perf/profile_creators/fast_navigation_profile_extender.py:31: > self._navigation_urls = self.NavigationUrls() > def GetUrlsToNavigate() > > https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... > tools/perf/profile_creators/fast_navigation_profile_extender.py:50: > self._queued_tabs = [] > What about making this a map from tabs to intial_urls? > > https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... > tools/perf/profile_creators/fast_navigation_profile_extender.py:53: > self._navigation_url_index = 0 > I don't like having this counter as a class's member. The more side-effect-free > methods, the easier it's to understand & unittest your implementation, and it's > less error prone. > > https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... > tools/perf/profile_creators/fast_navigation_profile_extender.py:62: > self._BrowserSetup(finder_options) > s/_BrowserSetup/_SetupBrowser > > https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... > tools/perf/profile_creators/fast_navigation_profile_extender.py:64: except: > you don't need the "except:\nraise:" > > https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... > tools/perf/profile_creators/fast_navigation_profile_extender.py:67: > self._BrowserTeardown() > s/_BrowserTeardown/_TeardownBrowser() nednguyen: PTAL
https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:31: self._navigation_urls = self.NavigationUrls() On 2015/02/09 21:35:02, nednguyen wrote: > def GetUrlsToNavigate() Done. https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:50: self._queued_tabs = [] On 2015/02/09 21:35:02, nednguyen wrote: > What about making this a map from tabs to intial_urls? That loses the ordering, which matters for performance. OrderedDict does what you want, but that requires python 2.7, and when I asked you, you couldn't remember the oldest python version that telemetry supports. https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:53: self._navigation_url_index = 0 On 2015/02/09 21:35:02, nednguyen wrote: > I don't like having this counter as a class's member. The more side-effect-free > methods, the easier it's to understand & unittest your implementation, and it's > less error prone. I've removed this member. https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:62: self._BrowserSetup(finder_options) On 2015/02/09 21:35:02, nednguyen wrote: > s/_BrowserSetup/_SetupBrowser I've renamed _BrowserSetup to _SetUpBrowser. https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:64: except: On 2015/02/09 21:35:02, nednguyen wrote: > you don't need the "except:\nraise:" Done. https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:67: self._BrowserTeardown() On 2015/02/09 21:35:02, nednguyen wrote: > s/_BrowserTeardown/_TeardownBrowser() I've renamed _BrowserTeardown to _TearDownBrowser.
https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:50: self._queued_tabs = [] On 2015/02/10 00:45:02, erikchen wrote: > On 2015/02/09 21:35:02, nednguyen wrote: > > What about making this a map from tabs to intial_urls? > > That loses the ordering, which matters for performance. OrderedDict does what > you want, but that requires python 2.7, and when I asked you, you couldn't > remember the oldest python version that telemetry supports. Can you elaborate on why the ordering matters for performance? https://codereview.chromium.org/907503002/diff/80001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/80001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:54: self._queued_tabs = [] I don't think you need this either. https://codereview.chromium.org/907503002/diff/80001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:105: initial_url = self._RetrieveTabUrl(tab) Not sure that you need to do this kind of checking. Note that Navigate's implementation does WaitForNavigate https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
nednguyen: PTAL https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:50: self._queued_tabs = [] On 2015/02/10 18:48:43, nednguyen wrote: > On 2015/02/10 00:45:02, erikchen wrote: > > On 2015/02/09 21:35:02, nednguyen wrote: > > > What about making this a map from tabs to intial_urls? > > > > That loses the ordering, which matters for performance. OrderedDict does what > > you want, but that requires python 2.7, and when I asked you, you couldn't > > remember the oldest python version that telemetry supports. > > Can you elaborate on why the ordering matters for performance? Sending commands through the telemetry/Chromium bindings takes time (BINDING_TIME). Let's say I'm simultaneously loading 10 tabs t1 through t10. I send them commands sequentially, so t1 is likely to finish loading first, and t10 likely to finish loading last. Let's say that I wait for the tabs to load in order t10 through t1. t10 takes the longest to load (let's say 5 seconds). Then I still need to check that t9 through t1 are loaded. Total time = 5s + 9 * (BINDING_TIME). If instead I check t1 through t10, it's likely that t1 through t9 will finish loading before t10 finishes. So Total time = 5s. All of this math assumes that t1 finishes loading first, and t10 finishes loading last, which will only be true in aggregate on average, not all of the time. There's no reason to turn an ordered list into an unordered dictionary, especially if there is a small performance penalty. https://codereview.chromium.org/907503002/diff/80001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/80001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:54: self._queued_tabs = [] On 2015/02/10 18:48:43, nednguyen wrote: > I don't think you need this either. Done. https://codereview.chromium.org/907503002/diff/80001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:105: initial_url = self._RetrieveTabUrl(tab) On 2015/02/10 18:48:43, nednguyen wrote: > Not sure that you need to do this kind of checking. Note that Navigate's > implementation does WaitForNavigate > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... This is necessary since I navigate the tab with 0 timeout, so the behavior is not well defined. (It may navigate, it may start navigating, or most likely, it may not even reflect that the navigation has started).
Let's add your runnable python script so we can actually run something with this patch. https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:50: self._queued_tabs = [] On 2015/02/10 21:30:11, erikchen wrote: > On 2015/02/10 18:48:43, nednguyen wrote: > > On 2015/02/10 00:45:02, erikchen wrote: > > > On 2015/02/09 21:35:02, nednguyen wrote: > > > > What about making this a map from tabs to intial_urls? > > > > > > That loses the ordering, which matters for performance. OrderedDict does > what > > > you want, but that requires python 2.7, and when I asked you, you couldn't > > > remember the oldest python version that telemetry supports. > > > > Can you elaborate on why the ordering matters for performance? > > Sending commands through the telemetry/Chromium bindings takes time > (BINDING_TIME). Let's say I'm simultaneously loading 10 tabs t1 through t10. I > send them commands sequentially, so t1 is likely to finish loading first, and > t10 likely to finish loading last. > > Let's say that I wait for the tabs to load in order t10 through t1. t10 takes > the longest to load (let's say 5 seconds). Then I still need to check that t9 > through t1 are loaded. > > Total time = 5s + 9 * (BINDING_TIME). > > If instead I check t1 through t10, it's likely that t1 through t9 will finish > loading before t10 finishes. So > > Total time = 5s. > > All of this math assumes that t1 finishes loading first, and t10 finishes > loading last, which will only be true in aggregate on average, not all of the > time. > > There's no reason to turn an ordered list into an unordered dictionary, > especially if there is a small performance penalty. I want to see real benchmark to justify whether this optimization matters. It's making the code more complicated. "Premature optimization is the root of all evil"
On 2015/02/10 22:11:38, nednguyen wrote: > Let's add your runnable python script so we can actually run something with this > patch. Doing so does not improve the clarity of the code I've written in this CL. It just starts to build into a Mega-CL, which I'd like to avoid. > > https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... > File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): > > https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... > tools/perf/profile_creators/fast_navigation_profile_extender.py:50: > self._queued_tabs = [] > On 2015/02/10 21:30:11, erikchen wrote: > > On 2015/02/10 18:48:43, nednguyen wrote: > > > On 2015/02/10 00:45:02, erikchen wrote: > > > > On 2015/02/09 21:35:02, nednguyen wrote: > > > > > What about making this a map from tabs to intial_urls? > > > > > > > > That loses the ordering, which matters for performance. OrderedDict does > > what > > > > you want, but that requires python 2.7, and when I asked you, you couldn't > > > > remember the oldest python version that telemetry supports. > > > > > > Can you elaborate on why the ordering matters for performance? > > > > Sending commands through the telemetry/Chromium bindings takes time > > (BINDING_TIME). Let's say I'm simultaneously loading 10 tabs t1 through t10. I > > send them commands sequentially, so t1 is likely to finish loading first, and > > t10 likely to finish loading last. > > > > Let's say that I wait for the tabs to load in order t10 through t1. t10 takes > > the longest to load (let's say 5 seconds). Then I still need to check that t9 > > through t1 are loaded. > > > > Total time = 5s + 9 * (BINDING_TIME). > > > > If instead I check t1 through t10, it's likely that t1 through t9 will finish > > loading before t10 finishes. So > > > > Total time = 5s. > > > > All of this math assumes that t1 finishes loading first, and t10 finishes > > loading last, which will only be true in aggregate on average, not all of the > > time. > > > > There's no reason to turn an ordered list into an unordered dictionary, > > especially if there is a small performance penalty. > > I want to see real benchmark to justify whether this optimization matters. It's > making the code more complicated. "Premature optimization is the root of all > evil"
https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:50: self._queued_tabs = [] On 2015/02/10 22:11:37, nednguyen wrote: > On 2015/02/10 21:30:11, erikchen wrote: > > On 2015/02/10 18:48:43, nednguyen wrote: > > > On 2015/02/10 00:45:02, erikchen wrote: > > > > On 2015/02/09 21:35:02, nednguyen wrote: > > > > > What about making this a map from tabs to intial_urls? > > > > > > > > That loses the ordering, which matters for performance. OrderedDict does > > what > > > > you want, but that requires python 2.7, and when I asked you, you couldn't > > > > remember the oldest python version that telemetry supports. > > > > > > Can you elaborate on why the ordering matters for performance? > > > > Sending commands through the telemetry/Chromium bindings takes time > > (BINDING_TIME). Let's say I'm simultaneously loading 10 tabs t1 through t10. I > > send them commands sequentially, so t1 is likely to finish loading first, and > > t10 likely to finish loading last. > > > > Let's say that I wait for the tabs to load in order t10 through t1. t10 takes > > the longest to load (let's say 5 seconds). Then I still need to check that t9 > > through t1 are loaded. > > > > Total time = 5s + 9 * (BINDING_TIME). > > > > If instead I check t1 through t10, it's likely that t1 through t9 will finish > > loading before t10 finishes. So > > > > Total time = 5s. > > > > All of this math assumes that t1 finishes loading first, and t10 finishes > > loading last, which will only be true in aggregate on average, not all of the > > time. > > > > There's no reason to turn an ordered list into an unordered dictionary, > > especially if there is a small performance penalty. > > I want to see real benchmark to justify whether this optimization matters. It's > making the code more complicated. "Premature optimization is the root of all > evil" There is no optimization here. I'm using a list of tuples. You want me to use an unordered dictionary. Using an unordered dictionary saves literally 0 lines of code, and has the cost of losing the existing ordering. There's no benefit to doing this. Don't quote aphorisms out of context.
Please answer the non style-nits question first before making further changes to fix style ones. https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:50: self._queued_tabs = [] On 2015/02/11 02:45:50, erikchen wrote: > On 2015/02/10 22:11:37, nednguyen wrote: > > On 2015/02/10 21:30:11, erikchen wrote: > > > On 2015/02/10 18:48:43, nednguyen wrote: > > > > On 2015/02/10 00:45:02, erikchen wrote: > > > > > On 2015/02/09 21:35:02, nednguyen wrote: > > > > > > What about making this a map from tabs to intial_urls? > > > > > > > > > > That loses the ordering, which matters for performance. OrderedDict does > > > what > > > > > you want, but that requires python 2.7, and when I asked you, you > couldn't > > > > > remember the oldest python version that telemetry supports. > > > > > > > > Can you elaborate on why the ordering matters for performance? > > > > > > Sending commands through the telemetry/Chromium bindings takes time > > > (BINDING_TIME). Let's say I'm simultaneously loading 10 tabs t1 through t10. > I > > > send them commands sequentially, so t1 is likely to finish loading first, > and > > > t10 likely to finish loading last. > > > > > > Let's say that I wait for the tabs to load in order t10 through t1. t10 > takes > > > the longest to load (let's say 5 seconds). Then I still need to check that > t9 > > > through t1 are loaded. > > > > > > Total time = 5s + 9 * (BINDING_TIME). > > > > > > If instead I check t1 through t10, it's likely that t1 through t9 will > finish > > > loading before t10 finishes. So > > > > > > Total time = 5s. > > > > > > All of this math assumes that t1 finishes loading first, and t10 finishes > > > loading last, which will only be true in aggregate on average, not all of > the > > > time. > > > > > > There's no reason to turn an ordered list into an unordered dictionary, > > > especially if there is a small performance penalty. > > > > I want to see real benchmark to justify whether this optimization matters. > It's > > making the code more complicated. "Premature optimization is the root of all > > evil" > > There is no optimization here. I'm using a list of tuples. You want me to use an > unordered dictionary. Using an unordered dictionary saves literally 0 lines of > code, and has the cost of losing the existing ordering. There's no benefit to > doing this. Don't quote aphorisms out of context. Hmh, there was context, but the context has changed. You was using _navigation_url_index to keep track of starting index in the queued list, which make the implementation hard to follow. I asked you to use the map so you can iterate through browser's tabs and use the map for getting tab's initial URL. Anyway, with the most updated patch, the queued list implementation is as good as map implementation. https://codereview.chromium.org/907503002/diff/80001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/80001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:105: initial_url = self._RetrieveTabUrl(tab) On 2015/02/10 21:30:11, erikchen wrote: > On 2015/02/10 18:48:43, nednguyen wrote: > > Not sure that you need to do this kind of checking. Note that Navigate's > > implementation does WaitForNavigate > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > This is necessary since I navigate the tab with 0 timeout, so the behavior is > not well defined. (It may navigate, it may start navigating, or most likely, it > may not even reflect that the navigation has started). If the behavior of timeout_in_seconds=0 is not well defined, I think we should improve the documentation in WebContent.Navigate(..) to reflect what happen when timeout_in_seconds is 0, and make sure that it does what you want. Maybe you can add an 'if' in InspectorPage.Navigate(...) to skip the wait when timeout is 0. https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:21: batch. Style nits: please make this docstring and others in this file follow the style in https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:40: # available tabs. Style nits: rather than comments, you can do: assert (self._NUM_PARALLEL_PAGES <= self._NUM_TABS, ' the batch size can't be larger than the number of available tabs.') which is self explanatory and make the assertion message much clearer. https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:111: # both exceptions. Should we close crashed tabs & reopen new tabs? https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:124: end_time = time.time() + self._PAGE_LOAD_TIMEOUT_IN_SECONDS Based on the implementation below, it looks like _PAGE_LOAD_TIMEOUT_IN_SECONDS is actually _TOTAL_PAGES_LOAD_TIMEOUT_IN_SECONDS_FOR_ONE_BATCH. This make the implementation a little more difficult. Do you have any reason why you do it this way rather than just make each tab in queued_tabs timeout after same amount of seconds? i.e st like: for tab, initial_url in queued_tabs: # wait until tab has navigated, time out after C seconds. https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:140: time.sleep(self._NAVIGATION_COMMIT_WAIT_IN_SECONDS) I think you should use tab.WaitForNavigate(timeout=self._NAVIGATION_COMMIT_WAIT_IN_SECONDS) here instead. Implementation of tab.WaitForNavigate is here: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... Basically it waits for the response from Page.frameNavigated to know whether the page has navigated. https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py:26: # pylint: disable=W0212 Nit: please use pylint: disable=access-protected... I can't recall this exactly but the new pylint allow you disable pylint with name, which is better. https://codereview.chromium.org/907503002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/profile_generator.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/profile_generator.py:36: if test_name.endswith('_creator'): Why this file change? I thought we both agree that we won't try to make this patch runnable?
https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:111: # both exceptions. On 2015/02/12 02:10:53, nednguyen wrote: > Should we close crashed tabs & reopen new tabs? Yes. But there's a whole can of worms that's going to be opened, which I don't want to do in this CL. Any time the method __getitem__ is called from inspector_backend_list.py, there is a probability that a python exception will be thrown (there's a race condition with devtools crashing). There's even logging to print a "helpful" message when the exception is about to be thrown: "About to explode...". Closing/reopening tabs is easy to implement, but is first dependent on fixing inspector_backend_list.py so that it doesn't non-deterministically crash. https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:124: end_time = time.time() + self._PAGE_LOAD_TIMEOUT_IN_SECONDS On 2015/02/12 02:10:53, nednguyen wrote: > Based on the implementation below, it looks like _PAGE_LOAD_TIMEOUT_IN_SECONDS > is actually _TOTAL_PAGES_LOAD_TIMEOUT_IN_SECONDS_FOR_ONE_BATCH. > > This make the implementation a little more difficult. Do you have any reason why > you do it this way rather than just make each tab in queued_tabs timeout after > same amount of seconds? > i.e st like: > for tab, initial_url in queued_tabs: > # wait until tab has navigated, time out after C seconds. Yes. One of the major points of this class is to batch the navigations to avoid paying the penalty for multiple timeouts. Let's say that my timeout is 15 seconds, and I have 10 times that will all timeout. Under the current implementation, after 15 seconds, the entire batch is skipped. With your suggestion, we would wait 150 seconds total. https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:140: time.sleep(self._NAVIGATION_COMMIT_WAIT_IN_SECONDS) On 2015/02/12 02:10:53, nednguyen wrote: > I think you should use > tab.WaitForNavigate(timeout=self._NAVIGATION_COMMIT_WAIT_IN_SECONDS) here > instead. > > Implementation of tab.WaitForNavigate is here: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > Basically it waits for the response from Page.frameNavigated to know whether the > page has navigated. That doesn't work. WaitForNavigate assumes that a navigation is in progress, which is not necessarily true. It's possible that a navigation has already finished. It's possible that the navigation hasn't even been committed yet. https://codereview.chromium.org/907503002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/profile_generator.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/profile_generator.py:36: if test_name.endswith('_creator'): On 2015/02/12 02:10:54, nednguyen wrote: > Why this file change? I thought we both agree that we won't try to make this > patch runnable? Without this change, small_profile_creator won't work, as the assertion will evaluate to false.
Patchset #7 (id:140001) has been deleted
nednguyen: PTAL. https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/40001/tools/perf/profile_creat... tools/perf/profile_creators/fast_navigation_profile_extender.py:50: self._queued_tabs = [] On 2015/02/12 02:10:53, nednguyen wrote: > On 2015/02/11 02:45:50, erikchen wrote: > > On 2015/02/10 22:11:37, nednguyen wrote: > > > On 2015/02/10 21:30:11, erikchen wrote: > > > > On 2015/02/10 18:48:43, nednguyen wrote: > > > > > On 2015/02/10 00:45:02, erikchen wrote: > > > > > > On 2015/02/09 21:35:02, nednguyen wrote: > > > > > > > What about making this a map from tabs to intial_urls? > > > > > > > > > > > > That loses the ordering, which matters for performance. OrderedDict > does > > > > what > > > > > > you want, but that requires python 2.7, and when I asked you, you > > couldn't > > > > > > remember the oldest python version that telemetry supports. > > > > > > > > > > Can you elaborate on why the ordering matters for performance? > > > > > > > > Sending commands through the telemetry/Chromium bindings takes time > > > > (BINDING_TIME). Let's say I'm simultaneously loading 10 tabs t1 through > t10. > > I > > > > send them commands sequentially, so t1 is likely to finish loading first, > > and > > > > t10 likely to finish loading last. > > > > > > > > Let's say that I wait for the tabs to load in order t10 through t1. t10 > > takes > > > > the longest to load (let's say 5 seconds). Then I still need to check that > > t9 > > > > through t1 are loaded. > > > > > > > > Total time = 5s + 9 * (BINDING_TIME). > > > > > > > > If instead I check t1 through t10, it's likely that t1 through t9 will > > finish > > > > loading before t10 finishes. So > > > > > > > > Total time = 5s. > > > > > > > > All of this math assumes that t1 finishes loading first, and t10 finishes > > > > loading last, which will only be true in aggregate on average, not all of > > the > > > > time. > > > > > > > > There's no reason to turn an ordered list into an unordered dictionary, > > > > especially if there is a small performance penalty. > > > > > > I want to see real benchmark to justify whether this optimization matters. > > It's > > > making the code more complicated. "Premature optimization is the root of all > > > evil" > > > > There is no optimization here. I'm using a list of tuples. You want me to use > an > > unordered dictionary. Using an unordered dictionary saves literally 0 lines of > > code, and has the cost of losing the existing ordering. There's no benefit to > > doing this. Don't quote aphorisms out of context. > > Hmh, there was context, but the context has changed. You was using > _navigation_url_index to keep track of starting index in the queued list, which > make the implementation hard to follow. I asked you to use the map so you can > iterate through browser's tabs and use the map for getting tab's initial URL. > Anyway, with the most updated patch, the queued list implementation is as good > as map implementation. Your statement "You was using _navigation_url_index to keep track of starting index in the queued list, which make the implementation hard to follow" is not accurate. If you look at this patch set, you'll see that _navigation_url_index is never indexed into _queued_tabs. Nothing is indexed into _queued_tabs. The three operations performed on _queued_tabs are: - append - iterate - clear Replacing _queued_tabs with a dictionary does not change any functionality or clarity, just the container. https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:21: batch. On 2015/02/12 02:10:53, nednguyen wrote: > Style nits: please make this docstring and others in this file follow the style > in https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments Done. I've updated all the formatting to be PEP compliant. https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:40: # available tabs. On 2015/02/12 02:10:53, nednguyen wrote: > > Style nits: rather than comments, you can do: > assert (self._NUM_PARALLEL_PAGES <= self._NUM_TABS, ' the batch size can't be > larger than the number of available tabs.') > > which is self explanatory and make the assertion message much clearer. Good point, done. https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py:26: # pylint: disable=W0212 On 2015/02/12 02:10:53, nednguyen wrote: > Nit: please use pylint: disable=access-protected... I can't recall this exactly > but the new pylint allow you disable pylint with name, which is better. Done.
https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:111: # both exceptions. On 2015/02/12 02:26:07, erikchen wrote: > On 2015/02/12 02:10:53, nednguyen wrote: > > Should we close crashed tabs & reopen new tabs? > > Yes. But there's a whole can of worms that's going to be opened, which I don't > want to do in this CL. > > Any time the method __getitem__ is called from inspector_backend_list.py, there > is a probability that a python exception will be thrown (there's a race > condition with devtools crashing). There's even logging to print a "helpful" > message when the exception is about to be thrown: "About to explode...". > > Closing/reopening tabs is easy to implement, but is first dependent on fixing > inspector_backend_list.py so that it doesn't non-deterministically crash. SGTM. We have a filed bug for that one: https://code.google.com/p/chromium/issues/detail?id=398467 here. It would require some major refactoring. https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:124: end_time = time.time() + self._PAGE_LOAD_TIMEOUT_IN_SECONDS On 2015/02/12 02:26:07, erikchen wrote: > On 2015/02/12 02:10:53, nednguyen wrote: > > Based on the implementation below, it looks like _PAGE_LOAD_TIMEOUT_IN_SECONDS > > is actually _TOTAL_PAGES_LOAD_TIMEOUT_IN_SECONDS_FOR_ONE_BATCH. > > > > This make the implementation a little more difficult. Do you have any reason > why > > you do it this way rather than just make each tab in queued_tabs timeout after > > same amount of seconds? > > i.e st like: > > for tab, initial_url in queued_tabs: > > # wait until tab has navigated, time out after C seconds. > > Yes. One of the major points of this class is to batch the navigations to avoid > paying the penalty for multiple timeouts. Let's say that my timeout is 15 > seconds, and I have 10 times that will all timeout. Under the current > implementation, after 15 seconds, the entire batch is skipped. With your > suggestion, we would wait 150 seconds total. Then why not timeout after each page after 1.5 second? https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:140: time.sleep(self._NAVIGATION_COMMIT_WAIT_IN_SECONDS) On 2015/02/12 02:26:07, erikchen wrote: > On 2015/02/12 02:10:53, nednguyen wrote: > > I think you should use > > tab.WaitForNavigate(timeout=self._NAVIGATION_COMMIT_WAIT_IN_SECONDS) here > > instead. > > > > Implementation of tab.WaitForNavigate is here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > Basically it waits for the response from Page.frameNavigated to know whether > the > > page has navigated. > > That doesn't work. WaitForNavigate assumes that a navigation is in progress, > which is not necessarily true. It's possible that a navigation has already > finished. It's possible that the navigation hasn't even been committed yet. Interesting. The case "navigation has already finished" sounds correct. But how can the case "navigation hasn't even been committed yet" happen? I remember that Navigate() is a blocking until it receives 'FrameId' from the devtool. Also, can we change WaitForNavigate so that if the first case happens, it would return immediately? https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py:69: self.assertEqual(count, expected_tab_navigation_count) Can we add a regression test that actually create a browser? If you need finder_options, you can use options = options_for_unittests.GetCopy() (see: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...) https://codereview.chromium.org/907503002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/profile_generator.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/profile_generator.py:36: if test_name.endswith('_creator'): On 2015/02/12 02:26:07, erikchen wrote: > On 2015/02/12 02:10:54, nednguyen wrote: > > Why this file change? I thought we both agree that we won't try to make this > > patch runnable? > > Without this change, small_profile_creator won't work, as the assertion will > evaluate to false. Does that mean it's currently broken? But why does the assertion fail anyway? Isn't small_profile_creator name ends with '_creator'?
https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:124: end_time = time.time() + self._PAGE_LOAD_TIMEOUT_IN_SECONDS On 2015/02/12 03:17:59, nednguyen wrote: > On 2015/02/12 02:26:07, erikchen wrote: > > On 2015/02/12 02:10:53, nednguyen wrote: > > > Based on the implementation below, it looks like > _PAGE_LOAD_TIMEOUT_IN_SECONDS > > > is actually _TOTAL_PAGES_LOAD_TIMEOUT_IN_SECONDS_FOR_ONE_BATCH. > > > > > > This make the implementation a little more difficult. Do you have any reason > > why > > > you do it this way rather than just make each tab in queued_tabs timeout > after > > > same amount of seconds? > > > i.e st like: > > > for tab, initial_url in queued_tabs: > > > # wait until tab has navigated, time out after C seconds. > > > > Yes. One of the major points of this class is to batch the navigations to > avoid > > paying the penalty for multiple timeouts. Let's say that my timeout is 15 > > seconds, and I have 10 times that will all timeout. Under the current > > implementation, after 15 seconds, the entire batch is skipped. With your > > suggestion, we would wait 150 seconds total. > > Then why not timeout after each page after 1.5 second? Because most pages requires longer than 1.5 seconds to load. Some pages might even load in 12 seconds. This logic provides the benefits of fast_navigation_profile_extender. Otherwise there's no need to have multiple tabs simultaneously loading pages. https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:140: time.sleep(self._NAVIGATION_COMMIT_WAIT_IN_SECONDS) On 2015/02/12 03:17:59, nednguyen wrote: > On 2015/02/12 02:26:07, erikchen wrote: > > On 2015/02/12 02:10:53, nednguyen wrote: > > > I think you should use > > > tab.WaitForNavigate(timeout=self._NAVIGATION_COMMIT_WAIT_IN_SECONDS) here > > > instead. > > > > > > Implementation of tab.WaitForNavigate is here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > > > Basically it waits for the response from Page.frameNavigated to know whether > > the > > > page has navigated. > > > > That doesn't work. WaitForNavigate assumes that a navigation is in progress, > > which is not necessarily true. It's possible that a navigation has already > > finished. It's possible that the navigation hasn't even been committed yet. > > Interesting. The case "navigation has already finished" sounds correct. But how > can the case "navigation hasn't even been committed yet" happen? I remember that > Navigate() is a blocking until it receives 'FrameId' from the devtool. > > Also, can we change WaitForNavigate so that if the first case happens, it would > return immediately? I spoke to quickly - you're right. Navigation hasn't been committed cannot happen. I've filed a bug https://code.google.com/p/chromium/issues/detail?id=457969 against myself to change WaitForNavigate to have the behavior you suggested.
https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:124: end_time = time.time() + self._PAGE_LOAD_TIMEOUT_IN_SECONDS On 2015/02/12 03:24:18, erikchen wrote: > On 2015/02/12 03:17:59, nednguyen wrote: > > On 2015/02/12 02:26:07, erikchen wrote: > > > On 2015/02/12 02:10:53, nednguyen wrote: > > > > Based on the implementation below, it looks like > > _PAGE_LOAD_TIMEOUT_IN_SECONDS > > > > is actually _TOTAL_PAGES_LOAD_TIMEOUT_IN_SECONDS_FOR_ONE_BATCH. > > > > > > > > This make the implementation a little more difficult. Do you have any > reason > > > why > > > > you do it this way rather than just make each tab in queued_tabs timeout > > after > > > > same amount of seconds? > > > > i.e st like: > > > > for tab, initial_url in queued_tabs: > > > > # wait until tab has navigated, time out after C seconds. > > > > > > Yes. One of the major points of this class is to batch the navigations to > > avoid > > > paying the penalty for multiple timeouts. Let's say that my timeout is 15 > > > seconds, and I have 10 times that will all timeout. Under the current > > > implementation, after 15 seconds, the entire batch is skipped. With your > > > suggestion, we would wait 150 seconds total. > > > > Then why not timeout after each page after 1.5 second? > > Because most pages requires longer than 1.5 seconds to load. Some pages might > even load in 12 seconds. This logic provides the benefits of > fast_navigation_profile_extender. Otherwise there's no need to have multiple > tabs simultaneously loading pages. Ok, this is legit. Maybe rename _PAGE_LOAD_TIMEOUT_IN_SECONDS to better reflecting that it's the total timeout amounts for each batch.
https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:124: end_time = time.time() + self._PAGE_LOAD_TIMEOUT_IN_SECONDS On 2015/02/12 03:24:18, erikchen wrote: > On 2015/02/12 03:17:59, nednguyen wrote: > > On 2015/02/12 02:26:07, erikchen wrote: > > > On 2015/02/12 02:10:53, nednguyen wrote: > > > > Based on the implementation below, it looks like > > _PAGE_LOAD_TIMEOUT_IN_SECONDS > > > > is actually _TOTAL_PAGES_LOAD_TIMEOUT_IN_SECONDS_FOR_ONE_BATCH. > > > > > > > > This make the implementation a little more difficult. Do you have any > reason > > > why > > > > you do it this way rather than just make each tab in queued_tabs timeout > > after > > > > same amount of seconds? > > > > i.e st like: > > > > for tab, initial_url in queued_tabs: > > > > # wait until tab has navigated, time out after C seconds. > > > > > > Yes. One of the major points of this class is to batch the navigations to > > avoid > > > paying the penalty for multiple timeouts. Let's say that my timeout is 15 > > > seconds, and I have 10 times that will all timeout. Under the current > > > implementation, after 15 seconds, the entire batch is skipped. With your > > > suggestion, we would wait 150 seconds total. > > > > Then why not timeout after each page after 1.5 second? > > Because most pages requires longer than 1.5 seconds to load. Some pages might > even load in 12 seconds. This logic provides the benefits of > fast_navigation_profile_extender. Otherwise there's no need to have multiple > tabs simultaneously loading pages. Ok, this is legit. Maybe rename _PAGE_LOAD_TIMEOUT_IN_SECONDS to better reflecting that it's the total timeout amounts for each batch.
On 2015/02/12 03:29:53, nednguyen wrote: > https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... > File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): > > https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... > tools/perf/profile_creators/fast_navigation_profile_extender.py:124: end_time = > time.time() + self._PAGE_LOAD_TIMEOUT_IN_SECONDS > On 2015/02/12 03:24:18, erikchen wrote: > > On 2015/02/12 03:17:59, nednguyen wrote: > > > On 2015/02/12 02:26:07, erikchen wrote: > > > > On 2015/02/12 02:10:53, nednguyen wrote: > > > > > Based on the implementation below, it looks like > > > _PAGE_LOAD_TIMEOUT_IN_SECONDS > > > > > is actually _TOTAL_PAGES_LOAD_TIMEOUT_IN_SECONDS_FOR_ONE_BATCH. > > > > > > > > > > This make the implementation a little more difficult. Do you have any > > reason > > > > why > > > > > you do it this way rather than just make each tab in queued_tabs timeout > > > after > > > > > same amount of seconds? > > > > > i.e st like: > > > > > for tab, initial_url in queued_tabs: > > > > > # wait until tab has navigated, time out after C seconds. > > > > > > > > Yes. One of the major points of this class is to batch the navigations to > > > avoid > > > > paying the penalty for multiple timeouts. Let's say that my timeout is 15 > > > > seconds, and I have 10 times that will all timeout. Under the current > > > > implementation, after 15 seconds, the entire batch is skipped. With your > > > > suggestion, we would wait 150 seconds total. > > > > > > Then why not timeout after each page after 1.5 second? > > > > Because most pages requires longer than 1.5 seconds to load. Some pages might > > even load in 12 seconds. This logic provides the benefits of > > fast_navigation_profile_extender. Otherwise there's no need to have multiple > > tabs simultaneously loading pages. > > Ok, this is legit. Maybe rename _PAGE_LOAD_TIMEOUT_IN_SECONDS to better > reflecting that it's the total timeout amounts for each batch. Good point, done.
https://codereview.chromium.org/907503002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/profile_generator.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/profile_generator.py:36: if test_name.endswith('_creator'): On 2015/02/12 03:17:59, nednguyen wrote: > On 2015/02/12 02:26:07, erikchen wrote: > > On 2015/02/12 02:10:54, nednguyen wrote: > > > Why this file change? I thought we both agree that we won't try to make this > > > patch runnable? > > > > Without this change, small_profile_creator won't work, as the assertion will > > evaluate to false. > > Does that mean it's currently broken? But why does the assertion fail anyway? > Isn't small_profile_creator name ends with '_creator'? The previous code goes through the profile_creator directory and asserts that all discovered files end with _creator. I'm adding new files to the directory that don't end in _creator.
https://codereview.chromium.org/907503002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/profile_generator.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/profile_generator.py:36: if test_name.endswith('_creator'): On 2015/02/12 03:38:35, erikchen wrote: > On 2015/02/12 03:17:59, nednguyen wrote: > > On 2015/02/12 02:26:07, erikchen wrote: > > > On 2015/02/12 02:10:54, nednguyen wrote: > > > > Why this file change? I thought we both agree that we won't try to make > this > > > > patch runnable? > > > > > > Without this change, small_profile_creator won't work, as the assertion will > > > evaluate to false. > > > > Does that mean it's currently broken? But why does the assertion fail anyway? > > Isn't small_profile_creator name ends with '_creator'? > > The previous code goes through the profile_creator directory and asserts that > all discovered files end with _creator. I'm adding new files to the directory > that don't end in _creator. I still don't understand. Line 30 shows that profile_creators_unfiltered should only contain classes whose base class is profile_creator.ProfileCreator. Your FastNavigationProfileExtender shouldn't be one of those. Or is the discover's implementation buggy here?
https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py:69: self.assertEqual(count, expected_tab_navigation_count) On 2015/02/12 03:17:59, nednguyen wrote: > Can we add a regression test that actually create a browser? If you need > finder_options, you can use options = options_for_unittests.GetCopy() (see: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...) Actually, I realize that you will need either to use wpr server or local server to make a regression test. If it's troublesome to set that up, feel free to skip this for now.
https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender_unittest.py:69: self.assertEqual(count, expected_tab_navigation_count) On 2015/02/12 16:53:56, nednguyen wrote: > On 2015/02/12 03:17:59, nednguyen wrote: > > Can we add a regression test that actually create a browser? If you need > > finder_options, you can use options = options_for_unittests.GetCopy() (see: > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...) > > Actually, I realize that you will need either to use wpr server or local server > to make a regression test. If it's troublesome to set that up, feel free to skip > this for now. What specifically are you trying to get from the regression test that uses a browser - just a basic test of functionality? If so, the class HistoryProfileExtender https://codereview.chromium.org/914253005/ is a perfect candidate, since it doesn't require any network access, and still generates a profile. I'd prefer to wait until that class exists before writing the regression test. https://codereview.chromium.org/907503002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/profile_generator.py (right): https://codereview.chromium.org/907503002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/profile_generator.py:36: if test_name.endswith('_creator'): On 2015/02/12 16:45:52, nednguyen wrote: > On 2015/02/12 03:38:35, erikchen wrote: > > On 2015/02/12 03:17:59, nednguyen wrote: > > > On 2015/02/12 02:26:07, erikchen wrote: > > > > On 2015/02/12 02:10:54, nednguyen wrote: > > > > > Why this file change? I thought we both agree that we won't try to make > > this > > > > > patch runnable? > > > > > > > > Without this change, small_profile_creator won't work, as the assertion > will > > > > evaluate to false. > > > > > > Does that mean it's currently broken? But why does the assertion fail > anyway? > > > Isn't small_profile_creator name ends with '_creator'? > > > > The previous code goes through the profile_creator directory and asserts that > > all discovered files end with _creator. I'm adding new files to the directory > > that don't end in _creator. > > I still don't understand. Line 30 shows that profile_creators_unfiltered should > only contain classes whose base class is profile_creator.ProfileCreator. Your > FastNavigationProfileExtender shouldn't be one of those. > > Or is the discover's implementation buggy here? The implementation is not buggy. This change is not actually required - I was mistaken. I've been ferrying patches between multiple CLs, and I managed to confuse myself.
lgtm. But please fix the arbitrary sleep before landing this. https://codereview.chromium.org/907503002/diff/200001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/200001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:144: time.sleep(self._NAVIGATION_COMMIT_WAIT_IN_SECONDS) Please wait for the WaitForNavigate CL landed & update this.
New patchsets have been uploaded after l-g-t-m from nednguyen@google.com
https://codereview.chromium.org/907503002/diff/200001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/200001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:144: time.sleep(self._NAVIGATION_COMMIT_WAIT_IN_SECONDS) On 2015/02/12 20:54:21, nednguyen wrote: > Please wait for the WaitForNavigate CL landed & update this. Done. The CL now uses WaitForNavigate(), and no longer needs to check the current/initial url of the tab.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/907503002/220001
The CQ bit was unchecked by erikchen@chromium.org
Patchset #10 (id:220001) has been deleted
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/907503002/200001
The CQ bit was unchecked by erikchen@chromium.org
Patchset #10 (id:240001) has been deleted
Patchset #10 (id:260001) has been deleted
Patchset #10 (id:280001) has been deleted
nednguyen: PTAL Diff patch set 10 against patch set 9 to see changes since your last review.
https://codereview.chromium.org/907503002/diff/300001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/300001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:91: def _WaitForUrlToChange(self, tab, initial_url, timeout): telemetry already has a util.Waitfor method that does exponential backoff: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... I suggest you use this instead of the implementation below.
https://codereview.chromium.org/907503002/diff/300001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/300001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:91: def _WaitForUrlToChange(self, tab, initial_url, timeout): On 2015/02/17 23:38:10, nednguyen wrote: > telemetry already has a util.Waitfor method that does exponential backoff: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > I suggest you use this instead of the implementation below. The function util.WaitFor() does not work well here. Its minimum sleep is 0.1s, and the sleep scales linearly up to 5.0s. These sleep durations are too large.
https://codereview.chromium.org/907503002/diff/300001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/300001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:91: def _WaitForUrlToChange(self, tab, initial_url, timeout): On 2015/02/17 23:46:10, erikchen wrote: > On 2015/02/17 23:38:10, nednguyen wrote: > > telemetry already has a util.Waitfor method that does exponential backoff: > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > I suggest you use this instead of the implementation below. > > The function util.WaitFor() does not work well here. Its minimum sleep is 0.1s, > and the sleep scales linearly up to 5.0s. These sleep durations are too large. You can making it configurable by making those parameters with default values.
https://codereview.chromium.org/907503002/diff/300001/tools/perf/profile_crea... File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): https://codereview.chromium.org/907503002/diff/300001/tools/perf/profile_crea... tools/perf/profile_creators/fast_navigation_profile_extender.py:91: def _WaitForUrlToChange(self, tab, initial_url, timeout): On 2015/02/17 23:50:10, nednguyen wrote: > On 2015/02/17 23:46:10, erikchen wrote: > > On 2015/02/17 23:38:10, nednguyen wrote: > > > telemetry already has a util.Waitfor method that does exponential backoff: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > > > I suggest you use this instead of the implementation below. > > > > The function util.WaitFor() does not work well here. Its minimum sleep is > 0.1s, > > and the sleep scales linearly up to 5.0s. These sleep durations are too large. > > You can making it configurable by making those parameters with default values. The conditional itself requires a dynamic variable: (self._RetrieveTabUrl() takes seconds_to_wait). While it is possible to use util.WaitFor() with configurable sleeps, it would still require dynamic calculation of seconds_to_wait. All of this is possible, but it would not make the code simpler, or easier to read.
On 2015/02/17 23:59:38, erikchen wrote: > https://codereview.chromium.org/907503002/diff/300001/tools/perf/profile_crea... > File tools/perf/profile_creators/fast_navigation_profile_extender.py (right): > > https://codereview.chromium.org/907503002/diff/300001/tools/perf/profile_crea... > tools/perf/profile_creators/fast_navigation_profile_extender.py:91: def > _WaitForUrlToChange(self, tab, initial_url, timeout): > On 2015/02/17 23:50:10, nednguyen wrote: > > On 2015/02/17 23:46:10, erikchen wrote: > > > On 2015/02/17 23:38:10, nednguyen wrote: > > > > telemetry already has a util.Waitfor method that does exponential backoff: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > > > > > I suggest you use this instead of the implementation below. > > > > > > The function util.WaitFor() does not work well here. Its minimum sleep is > > 0.1s, > > > and the sleep scales linearly up to 5.0s. These sleep durations are too > large. > > > > You can making it configurable by making those parameters with default values. > > The conditional itself requires a dynamic variable: (self._RetrieveTabUrl() > takes seconds_to_wait). > > While it is possible to use util.WaitFor() with configurable sleeps, it would > still require dynamic calculation of seconds_to_wait. All of this is possible, > but it would not make the code simpler, or easier to read. Ok. We can land this and iterate on the specific implementation later. LGTM
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/907503002/300001
Message was sent while issue was closed.
Committed patchset #10 (id:300001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/16b5175f8967c8ea8430f7ada22eeb8586776f9a Cr-Commit-Position: refs/heads/master@{#316738} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
