Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13328004:
Telemetry tab_switching_test. (Closed)
|
Created:
7 years, 8 months ago by shatch Modified:
7 years, 6 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, Danh Nguyen, jbauman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionPorted tab_switching_test.cc to telemetry.
BUG=
NOTRY=true
Patch Set 1 #
Total comments: 1
Patch Set 2 : Removed var. #
Total comments: 12
Patch Set 3 : Changes from review. #
Total comments: 6
Patch Set 4 : Changes from review. #Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/13328004/diff/1/tools/perf/perf_tools/tab_swi... File tools/perf/perf_tools/tab_switching_benchmark.py (right): https://codereview.chromium.org/13328004/diff/1/tools/perf/perf_tools/tab_swi... tools/perf/perf_tools/tab_switching_benchmark.py:31: # These pages seem to be broken Should these be replaced with new pages?
Yay for another test moved into telemetry! A few questions/comments inline. https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... File tools/perf/perf_tools/tab_switching_benchmark.py (right): https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:8: import time Style nit: put system imports before telemetry imports https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:22: for h in TAB_SWITCHING_HISTOGRAMS] I know this is copy/paste from another benchmark, but I still think it would be nice if this were not an array since there is only one. self.histogram = histgram_measurement.HistogramMeasurement({'name': 'MPArch.RWH_TabSwitchPaintDuration', 'units': ''}, histogram_measurement.BROWSER_HISTOGRAM) https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:32: #"kannada.chakradeo.net", "ml.wikipedia.org" I'd just omit these https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:44: tabs[i].Navigate(launch_url) What is the point of navigating to the launch URL and then redirecting to the target page? Why not just navigate to the target page? https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:50: while t.EvaluateJavaScript('document.webkitHidden') != False: Recommend using WaitFor() here. https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:52: time.sleep(0.1) Why sleep? Is this what the old test did?
On 2013/03/29 20:37:35, tonyg wrote: > Yay for another test moved into telemetry! A few questions/comments inline. > > https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... > File tools/perf/perf_tools/tab_switching_benchmark.py (right): > > https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... > tools/perf/perf_tools/tab_switching_benchmark.py:8: import time > Style nit: put system imports before telemetry imports > > https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... > tools/perf/perf_tools/tab_switching_benchmark.py:22: for h in > TAB_SWITCHING_HISTOGRAMS] > I know this is copy/paste from another benchmark, but I still think it would be > nice if this were not an array since there is only one. > > self.histogram = histgram_measurement.HistogramMeasurement({'name': > 'MPArch.RWH_TabSwitchPaintDuration', 'units': ''}, > histogram_measurement.BROWSER_HISTOGRAM) > > https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... > tools/perf/perf_tools/tab_switching_benchmark.py:32: #"kannada.chakradeo.net", > "ml.wikipedia.org" > I'd just omit these > > https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... > tools/perf/perf_tools/tab_switching_benchmark.py:44: > tabs[i].Navigate(launch_url) > What is the point of navigating to the launch URL and then redirecting to the > target page? Why not just navigate to the target page? > Think it was just a leftover from when I was trying to figure out how to navigate the tabs properly. > https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... > tools/perf/perf_tools/tab_switching_benchmark.py:50: while > t.EvaluateJavaScript('document.webkitHidden') != False: > Recommend using WaitFor() here. > > https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... > tools/perf/perf_tools/tab_switching_benchmark.py:52: time.sleep(0.1) > Why sleep? Is this what the old test did? Think it was just a leftover from when I was trying to figure out why the histogram wasn't showing up.
New snapshot uploaded. https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... File tools/perf/perf_tools/tab_switching_benchmark.py (right): https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:8: import time On 2013/03/29 20:37:35, tonyg wrote: > Style nit: put system imports before telemetry imports Done. https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:22: for h in TAB_SWITCHING_HISTOGRAMS] On 2013/03/29 20:37:35, tonyg wrote: > I know this is copy/paste from another benchmark, but I still think it would be > nice if this were not an array since there is only one. > > self.histogram = histgram_measurement.HistogramMeasurement({'name': > 'MPArch.RWH_TabSwitchPaintDuration', 'units': ''}, > histogram_measurement.BROWSER_HISTOGRAM) Done. https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:32: #"kannada.chakradeo.net", "ml.wikipedia.org" On 2013/03/29 20:37:35, tonyg wrote: > I'd just omit these Done. https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:44: tabs[i].Navigate(launch_url) On 2013/03/29 20:37:35, tonyg wrote: > What is the point of navigating to the launch URL and then redirecting to the > target page? Why not just navigate to the target page? Done. https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:50: while t.EvaluateJavaScript('document.webkitHidden') != False: On 2013/03/29 20:37:35, tonyg wrote: > Recommend using WaitFor() here. Done. https://codereview.chromium.org/13328004/diff/2001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:52: time.sleep(0.1) On 2013/03/29 20:37:35, tonyg wrote: > Why sleep? Is this what the old test did? Done.
A few last nits. Everything else looks good to me, but I'd be curious to get Nat or Dave's opinion here before landing. They might want this to be an action or something. https://codereview.chromium.org/13328004/diff/9001/tools/perf/page_sets/tab_s... File tools/perf/page_sets/tab_switching.json (right): https://codereview.chromium.org/13328004/diff/9001/tools/perf/page_sets/tab_s... tools/perf/page_sets/tab_switching.json:5: "url": "file:///blank_page.html", This is gone now. Should it be "about:blank"? Or should it be the first URL in the list of tabs to open? https://codereview.chromium.org/13328004/diff/9001/tools/perf/page_sets/tab_s... tools/perf/page_sets/tab_switching.json:6: "serving_dirs": [".", "../../../data/tab_switching"] You can drop "." now https://codereview.chromium.org/13328004/diff/9001/tools/perf/perf_tools/tab_... File tools/perf/perf_tools/tab_switching_benchmark.py (right): https://codereview.chromium.org/13328004/diff/9001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:11: {'name': 'MPArch.RWH_TabSwitchPaintDuration', 'units': ''},] This is unused now https://codereview.chromium.org/13328004/diff/9001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:45: return bool(not t.EvaluateJavaScript('document.webkitHidden')) Either the bool is unnecessary or the not is in the wrong place.
On 2013/03/29 22:14:17, tonyg wrote: > A few last nits. Everything else looks good to me, but I'd be curious to get Nat > or Dave's opinion here before landing. They might want this to be an action or > something. > > https://codereview.chromium.org/13328004/diff/9001/tools/perf/page_sets/tab_s... > File tools/perf/page_sets/tab_switching.json (right): > > https://codereview.chromium.org/13328004/diff/9001/tools/perf/page_sets/tab_s... > tools/perf/page_sets/tab_switching.json:5: "url": "file:///blank_page.html", > This is gone now. Should it be "about:blank"? Or should it be the first URL in > the list of tabs to open? > about:blank doesn't seem to work, and my thought was putting one url here and the rest elsewhere wasn't very clean. > https://codereview.chromium.org/13328004/diff/9001/tools/perf/page_sets/tab_s... > tools/perf/page_sets/tab_switching.json:6: "serving_dirs": [".", > "../../../data/tab_switching"] > You can drop "." now > > https://codereview.chromium.org/13328004/diff/9001/tools/perf/perf_tools/tab_... > File tools/perf/perf_tools/tab_switching_benchmark.py (right): > > https://codereview.chromium.org/13328004/diff/9001/tools/perf/perf_tools/tab_... > tools/perf/perf_tools/tab_switching_benchmark.py:11: {'name': > 'MPArch.RWH_TabSwitchPaintDuration', 'units': ''},] > This is unused now > > https://codereview.chromium.org/13328004/diff/9001/tools/perf/perf_tools/tab_... > tools/perf/perf_tools/tab_switching_benchmark.py:45: return bool(not > t.EvaluateJavaScript('document.webkitHidden')) > Either the bool is unnecessary or the not is in the wrong place.
https://codereview.chromium.org/13328004/diff/9001/tools/perf/page_sets/tab_s... > > tools/perf/page_sets/tab_switching.json:5: "url": "file:///blank_page.html", > > This is gone now. Should it be "about:blank"? Or should it be the first URL in > > the list of tabs to open? > > > > about:blank doesn't seem to work, and my thought was putting one url here and > the rest elsewhere wasn't very clean. OK
New snapshot uploaded. https://codereview.chromium.org/13328004/diff/9001/tools/perf/perf_tools/tab_... File tools/perf/perf_tools/tab_switching_benchmark.py (right): https://codereview.chromium.org/13328004/diff/9001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:11: {'name': 'MPArch.RWH_TabSwitchPaintDuration', 'units': ''},] On 2013/03/29 22:14:17, tonyg wrote: > This is unused now Done. https://codereview.chromium.org/13328004/diff/9001/tools/perf/perf_tools/tab_... tools/perf/perf_tools/tab_switching_benchmark.py:45: return bool(not t.EvaluateJavaScript('document.webkitHidden')) On 2013/03/29 22:14:17, tonyg wrote: > Either the bool is unnecessary or the not is in the wrong place. Done.
Remind me what the tab switching test used to do, and what we plan it to do in the future? @danhn was doing something here, and @jbauman is working on API for correctly obtaining tab switching times [they're going to be a blatant lie right now, I think?]
(https://codereview.chromium.org/11668013/ being the related review)
On 2013/04/01 00:13:43, nduca wrote: > (https://codereview.chromium.org/11668013/ being the related review) The test loads a set of pages into tabs, then cycles through them in order and grabs the histogram for "MPArch.RWH_TabSwitchPaintDuration". I believe that metric is the time from RenderWidget::WasShown is called until the first paint is finished. I don't know what the future plans for the test are. Perhaps Tony can give some more insight.
On 2013/04/01 21:46:07, shatch wrote: > On 2013/04/01 00:13:43, nduca wrote: > > (https://codereview.chromium.org/11668013/ being the related review) > > The test loads a set of pages into tabs, then cycles through them in order and > grabs the histogram for "MPArch.RWH_TabSwitchPaintDuration". I believe that > metric is the time from RenderWidget::WasShown is called until the first paint > is finished. I don't know what the future plans for the test are. Perhaps Tony > can give some more insight. This test is pretty similar to the one nduca linked above. https://codereview.chromium.org/11668013/ Would it make sense to consolidate these tests? That one is also using Telemetry at the core level instead of at the page level, which seems to make more sense in this case, instead of using a dummy page set.
The test in https://codereview.chromium.**org/11668013/<https://codereview.chromium.org/1... is part of the stress tests which Vangelis would like to have: 1. Open up multiple tabs within a window, cycle through them in a random order, close them in a random order, make sure that everything works and memory is released. 2. Open up multiple window with multiple tabs, do stuff similar to #1 3. Stress test window resizing by opening up multiple tabs in a single window and continuously resize the window for a while. Check for hangs, memory leaks. While this test seems to check on tab switching time, so I guess they're different. Thanks, Danh On Tue, Apr 2, 2013 at 8:40 PM, <dtu@chromium.org> wrote: > On 2013/04/01 21:46:07, shatch wrote: > >> On 2013/04/01 00:13:43, nduca wrote: >> > (https://codereview.chromium.**org/11668013/<https://codereview.chromium.org/1... the related review) >> > > The test loads a set of pages into tabs, then cycles through them in >> order and >> grabs the histogram for "MPArch.RWH_**TabSwitchPaintDuration". I believe >> that >> metric is the time from RenderWidget::WasShown is called until the first >> paint >> is finished. I don't know what the future plans for the test are. Perhaps >> Tony >> can give some more insight. >> > > This test is pretty similar to the one nduca linked above. > https://codereview.chromium.**org/11668013/<https://codereview.chromium.org/1... > > Would it make sense to consolidate these tests? That one is also using > Telemetry > at the core level instead of at the page level, which seems to make more > sense > in this case, instead of using a dummy page set. > > https://codereview.chromium.**org/13328004/<https://codereview.chromium.org/1... >
Could we just add measurements to the stress test described by Danh? It is already cycling through tabs in a random order and closing tabs in a random order. We could just measure tab switch and tab close time in addition to memory. Thoughts? On Wed, Apr 3, 2013 at 8:34 AM, Danh Nguyen <danhn@google.com> wrote: > The test in https://codereview.chromium.**org/11668013/<https://codereview.chromium.org/1... is > part of the stress tests which Vangelis would like to have: > > 1. Open up multiple tabs within a window, cycle through them in a random > order, close them in a random order, make sure that everything works and > memory is released. > 2. Open up multiple window with multiple tabs, do stuff similar to #1 > 3. Stress test window resizing by opening up multiple tabs in a single > window and continuously resize the window for a while. Check for hangs, > memory leaks. > > While this test seems to check on tab switching time, so I guess they're > different. > > Thanks, > Danh > > > On Tue, Apr 2, 2013 at 8:40 PM, <dtu@chromium.org> wrote: > >> On 2013/04/01 21:46:07, shatch wrote: >> >>> On 2013/04/01 00:13:43, nduca wrote: >>> > (https://codereview.chromium.**org/11668013/<https://codereview.chromium.org/1... the related review) >>> >> >> The test loads a set of pages into tabs, then cycles through them in >>> order and >>> grabs the histogram for "MPArch.RWH_**TabSwitchPaintDuration". I >>> believe that >>> metric is the time from RenderWidget::WasShown is called until the first >>> paint >>> is finished. I don't know what the future plans for the test are. >>> Perhaps Tony >>> can give some more insight. >>> >> >> This test is pretty similar to the one nduca linked above. >> https://codereview.chromium.**org/11668013/<https://codereview.chromium.org/1... >> >> Would it make sense to consolidate these tests? That one is also using >> Telemetry >> at the core level instead of at the page level, which seems to make more >> sense >> in this case, instead of using a dummy page set. >> >> https://codereview.chromium.**org/13328004/<https://codereview.chromium.org/1... >> > >
Yeah, they're fairly similar, don't see why we can't just merge them. One note about the other test, the call to Tab::Activate is asynchonous (I believe?), so quickly iterating the list and calling Activate() may not give each one time to become visible. I don't know if that's relevant to the gpu stress test, but I don't think the MPArch.RWH_TabSwitchPaintDuration metric will be valid unless you wait. On Wed, Apr 3, 2013 at 9:29 AM, Rebecca Crabb <rcrabb@google.com> wrote: > Could we just add measurements to the stress test described by Danh? It > is already cycling through tabs in a random order and closing tabs in a > random order. We could just measure tab switch and tab close time in > addition to memory. Thoughts? > > > On Wed, Apr 3, 2013 at 8:34 AM, Danh Nguyen <danhn@google.com> wrote: > >> The test in https://codereview.chromium.**org/11668013/<https://codereview.chromium.org/1... is >> part of the stress tests which Vangelis would like to have: >> >> 1. Open up multiple tabs within a window, cycle through them in a random >> order, close them in a random order, make sure that everything works and >> memory is released. >> 2. Open up multiple window with multiple tabs, do stuff similar to #1 >> 3. Stress test window resizing by opening up multiple tabs in a single >> window and continuously resize the window for a while. Check for hangs, >> memory leaks. >> >> While this test seems to check on tab switching time, so I guess they're >> different. >> >> Thanks, >> Danh >> >> >> On Tue, Apr 2, 2013 at 8:40 PM, <dtu@chromium.org> wrote: >> >>> On 2013/04/01 21:46:07, shatch wrote: >>> >>>> On 2013/04/01 00:13:43, nduca wrote: >>>> > (https://codereview.chromium.**org/11668013/<https://codereview.chromium.org/1... the related review) >>>> >>> >>> The test loads a set of pages into tabs, then cycles through them in >>>> order and >>>> grabs the histogram for "MPArch.RWH_**TabSwitchPaintDuration". I >>>> believe that >>>> metric is the time from RenderWidget::WasShown is called until the >>>> first paint >>>> is finished. I don't know what the future plans for the test are. >>>> Perhaps Tony >>>> can give some more insight. >>>> >>> >>> This test is pretty similar to the one nduca linked above. >>> https://codereview.chromium.**org/11668013/<https://codereview.chromium.org/1... >>> >>> Would it make sense to consolidate these tests? That one is also using >>> Telemetry >>> at the core level instead of at the page level, which seems to make more >>> sense >>> in this case, instead of using a dummy page set. >>> >>> https://codereview.chromium.**org/13328004/<https://codereview.chromium.org/1... >>> >> >> >
I can't help wonder if theres a session concept that is shared between these two tests. E.g. telemetry, create a page with a full session. Then there are tests that use that session in different ways, e.g. switching I also share Simon's concerns about activate being async. We should get some bugs filed about the tabswitchpaintduration metric being busted [cc jbauman on it]. We should also think careflly through whether we're doing enough "stressing" in the tab switching test. On Wed, Apr 3, 2013 at 9:59 AM, Simon Hatch <simonhatch@google.com> wrote: > Yeah, they're fairly similar, don't see why we can't just merge them. > > One note about the other test, the call to Tab::Activate is asynchonous (I > believe?), so quickly iterating the list and calling Activate() may not > give each one time to become visible. I don't know if that's relevant to > the gpu stress test, but I don't think the > MPArch.RWH_TabSwitchPaintDuration metric will be valid unless you wait. > > > On Wed, Apr 3, 2013 at 9:29 AM, Rebecca Crabb <rcrabb@google.com> wrote: > >> Could we just add measurements to the stress test described by Danh? It >> is already cycling through tabs in a random order and closing tabs in a >> random order. We could just measure tab switch and tab close time in >> addition to memory. Thoughts? >> >> >> On Wed, Apr 3, 2013 at 8:34 AM, Danh Nguyen <danhn@google.com> wrote: >> >>> The test in https://codereview.chromium.**org/11668013/<https://codereview.chromium.org/1... is >>> part of the stress tests which Vangelis would like to have: >>> >>> 1. Open up multiple tabs within a window, cycle through them in a random >>> order, close them in a random order, make sure that everything works and >>> memory is released. >>> 2. Open up multiple window with multiple tabs, do stuff similar to #1 >>> 3. Stress test window resizing by opening up multiple tabs in a single >>> window and continuously resize the window for a while. Check for hangs, >>> memory leaks. >>> >>> While this test seems to check on tab switching time, so I guess they're >>> different. >>> >>> Thanks, >>> Danh >>> >>> >>> On Tue, Apr 2, 2013 at 8:40 PM, <dtu@chromium.org> wrote: >>> >>>> On 2013/04/01 21:46:07, shatch wrote: >>>> >>>>> On 2013/04/01 00:13:43, nduca wrote: >>>>> > (https://codereview.chromium.**org/11668013/<https://codereview.chromium.org/1... the related review) >>>>> >>>> >>>> The test loads a set of pages into tabs, then cycles through them in >>>>> order and >>>>> grabs the histogram for "MPArch.RWH_**TabSwitchPaintDuration". I >>>>> believe that >>>>> metric is the time from RenderWidget::WasShown is called until the >>>>> first paint >>>>> is finished. I don't know what the future plans for the test are. >>>>> Perhaps Tony >>>>> can give some more insight. >>>>> >>>> >>>> This test is pretty similar to the one nduca linked above. >>>> https://codereview.chromium.**org/11668013/<https://codereview.chromium.org/1... >>>> >>>> Would it make sense to consolidate these tests? That one is also using >>>> Telemetry >>>> at the core level instead of at the page level, which seems to make >>>> more sense >>>> in this case, instead of using a dummy page set. >>>> >>>> https://codereview.chromium.**org/13328004/<https://codereview.chromium.org/1... >>>> >>> >>> >> > |
