Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1152763002:
Fix tab_switching measurement bug. (Closed)
DescriptionFix tab_switching measurement bug.
_IsDone is always true, which makes it activate next tab regardless if the tab is rendered. It should wait until a reading of RWH_TabSwitchPaintDuration is obtained before switching to next tab. Also, it should pass each tab to GetHistogram(), not always the last tab. Unittest added. (./run_tests measurements.tab_switching).
BUG=None
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect
Committed: https://crrev.com/2eee3f91209b8ccf16e7f64c03d42fbafd724ebe
Cr-Commit-Position: refs/heads/master@{#337191}
Patch Set 1 #Patch Set 2 : Unittest added #
Total comments: 2
Patch Set 3 : #
Total comments: 6
Patch Set 4 : Update unittest #
Total comments: 3
Patch Set 5 : rebase and revert parameter name change #Patch Set 6 : Fix import error #Patch Set 7 : Fix benchmark_smoke_unittest failure because tab_switching failed to switch tab if there's only one… #
Total comments: 1
Patch Set 8 : Fix broken unittest on Chrome #Patch Set 9 : Rebase #
Messages
Total messages: 62 (25 generated)
deanliao@chromium.org changed reviewers: + nednguyen@google.com
Can you add a unittest that will fail without your fix?
On 2015/05/22 16:07:17, nednguyen wrote: > Can you add a unittest that will fail without your fix? Back from vacation. Thanks for your suggestion. By adding a unittest, I found another bug that inside tab loop, it should pass each tab instead of last tab into GetHistogram().
https://codereview.chromium.org/1152763002/diff/20001/tools/perf/measurements... File tools/perf/measurements/tab_switching.py (right): https://codereview.chromium.org/1152763002/diff/20001/tools/perf/measurements... tools/perf/measurements/tab_switching.py:86: for tab in browser.tabs: IIRC, browser.tabs 's __iter__ doesn't actually return the tab. https://code.google.com/p/chromium/issues/detail?id=473202
On 2015/06/15 23:24:55, nednguyen wrote: > https://codereview.chromium.org/1152763002/diff/20001/tools/perf/measurements... > File tools/perf/measurements/tab_switching.py (right): > > https://codereview.chromium.org/1152763002/diff/20001/tools/perf/measurements... > tools/perf/measurements/tab_switching.py:86: for tab in browser.tabs: > IIRC, browser.tabs 's __iter__ doesn't actually return the tab. > > https://code.google.com/p/chromium/issues/detail?id=473202 You are right. Fixed.
https://codereview.chromium.org/1152763002/diff/20001/tools/perf/measurements... File tools/perf/measurements/tab_switching.py (right): https://codereview.chromium.org/1152763002/diff/20001/tools/perf/measurements... tools/perf/measurements/tab_switching.py:86: for tab in browser.tabs: On 2015/06/15 23:24:55, nednguyen wrote: > IIRC, browser.tabs 's __iter__ doesn't actually return the tab. > > https://code.google.com/p/chromium/issues/detail?id=473202 Done.
nednguyen@google.com changed reviewers: + eakuefner@chromium.org
+Ethan
https://codereview.chromium.org/1152763002/diff/40001/tools/perf/measurements... File tools/perf/measurements/tab_switching_unittest.py (right): https://codereview.chromium.org/1152763002/diff/40001/tools/perf/measurements... tools/perf/measurements/tab_switching_unittest.py:16: class FakePlatform(object): Since you're already importing the mock library, can't you also use MagicMock to replace all of these handwritten mocks? Also, minor nit: I think 'Fake' is the wrong term here; I like Fowler's summary of the difference between dummy/fake/mock/stub in this article: http://martinfowler.com/articles/mocksArentStubs.html . But you can also avoid these quibbles by using the *ForTest convention that we use in other Telemetry unit tests. https://codereview.chromium.org/1152763002/diff/40001/tools/perf/measurements... tools/perf/measurements/tab_switching_unittest.py:32: """Used to mock a browser tab.""" nit: Why docstring here? It seems like this is as self-explanatory as the rest of the Fake* classes. https://codereview.chromium.org/1152763002/diff/40001/tools/perf/measurements... tools/perf/measurements/tab_switching_unittest.py:63: # For sanity check: #tabs == #page_sets Should be #tabs == #pages, right?
Thanks for the review. https://codereview.chromium.org/1152763002/diff/40001/tools/perf/measurements... File tools/perf/measurements/tab_switching_unittest.py (right): https://codereview.chromium.org/1152763002/diff/40001/tools/perf/measurements... tools/perf/measurements/tab_switching_unittest.py:16: class FakePlatform(object): On 2015/06/18 17:23:29, eakuefner wrote: > Since you're already importing the mock library, can't you also use MagicMock to > replace all of these handwritten mocks? > > Also, minor nit: I think 'Fake' is the wrong term here; I like Fowler's summary > of the difference between dummy/fake/mock/stub in this article: > http://martinfowler.com/articles/mocksArentStubs.html . But you can also avoid > these quibbles by using the *ForTest convention that we use in other Telemetry > unit tests. Except Browser and PageSet, I use MagicMock for the rest classes. Also, I follow *ForTest convention now. https://codereview.chromium.org/1152763002/diff/40001/tools/perf/measurements... tools/perf/measurements/tab_switching_unittest.py:32: """Used to mock a browser tab.""" On 2015/06/18 17:23:29, eakuefner wrote: > nit: Why docstring here? It seems like this is as self-explanatory as the rest > of the Fake* classes. Removed. https://codereview.chromium.org/1152763002/diff/40001/tools/perf/measurements... tools/perf/measurements/tab_switching_unittest.py:63: # For sanity check: #tabs == #page_sets On 2015/06/18 17:23:29, eakuefner wrote: > Should be #tabs == #pages, right? Done.
On 2015/06/22 06:37:08, deanliao wrote: > Thanks for the review. > > https://codereview.chromium.org/1152763002/diff/40001/tools/perf/measurements... > File tools/perf/measurements/tab_switching_unittest.py (right): > > https://codereview.chromium.org/1152763002/diff/40001/tools/perf/measurements... > tools/perf/measurements/tab_switching_unittest.py:16: class > FakePlatform(object): > On 2015/06/18 17:23:29, eakuefner wrote: > > Since you're already importing the mock library, can't you also use MagicMock > to > > replace all of these handwritten mocks? > > > > Also, minor nit: I think 'Fake' is the wrong term here; I like Fowler's > summary > > of the difference between dummy/fake/mock/stub in this article: > > http://martinfowler.com/articles/mocksArentStubs.html . But you can also avoid > > these quibbles by using the *ForTest convention that we use in other Telemetry > > unit tests. > > Except Browser and PageSet, I use MagicMock for the rest classes. Also, I follow > *ForTest convention now. > > https://codereview.chromium.org/1152763002/diff/40001/tools/perf/measurements... > tools/perf/measurements/tab_switching_unittest.py:32: """Used to mock a browser > tab.""" > On 2015/06/18 17:23:29, eakuefner wrote: > > nit: Why docstring here? It seems like this is as self-explanatory as the rest > > of the Fake* classes. > > Removed. > > https://codereview.chromium.org/1152763002/diff/40001/tools/perf/measurements... > tools/perf/measurements/tab_switching_unittest.py:63: # For sanity check: #tabs > == #page_sets > On 2015/06/18 17:23:29, eakuefner wrote: > > Should be #tabs == #pages, right? > > Done. ping
lgtm
https://codereview.chromium.org/1152763002/diff/60001/tools/perf/measurements... File tools/perf/measurements/tab_switching.py (right): https://codereview.chromium.org/1152763002/diff/60001/tools/perf/measurements... tools/perf/measurements/tab_switching.py:63: def ValidateAndMeasurePage(self, page, last_tab, results): Why change the param from tab to last_tab? You don't want to change the param name of overrided method in python because the callsite may do s.t like this: test.ValidateAndMeasurePage(page=this_page, tab=this_tab, results=results) https://codereview.chromium.org/1152763002/diff/60001/tools/perf/measurements... tools/perf/measurements/tab_switching.py:86: for i in xrange(len(browser.tabs)): rebase this since the patch to fix the API is landed?
https://codereview.chromium.org/1152763002/diff/60001/tools/perf/measurements... File tools/perf/measurements/tab_switching.py (right): https://codereview.chromium.org/1152763002/diff/60001/tools/perf/measurements... tools/perf/measurements/tab_switching.py:63: def ValidateAndMeasurePage(self, page, last_tab, results): On 2015/06/23 16:56:20, nednguyen wrote: > Why change the param from tab to last_tab? You don't want to change the param > name of overrided method in python because the callsite may do s.t like this: > > test.ValidateAndMeasurePage(page=this_page, tab=this_tab, results=results) Sorry, I didn't consider the side effect. Reverted
The CQ bit was checked by deanliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org Link to the patchset: https://codereview.chromium.org/1152763002/#ps80001 (title: "rebase and revert parameter name change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152763002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by nednguyen@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152763002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by deanliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1152763002/#ps100001 (title: "Fix import error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152763002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152763002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by deanliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152763002/100001
On 2015/06/24 07:55:02, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1152763002/100001 Hi all, I found that tab_switching fix breaks benchmark_smoke_test as it only gives single page to tab_switching, which cause it fails to switch tab. Please check if my fix is reasonable.
https://codereview.chromium.org/1152763002/diff/120001/tools/perf/benchmarks/... File tools/perf/benchmarks/benchmark_smoke_unittest.py (right): https://codereview.chromium.org/1152763002/diff/120001/tools/perf/benchmarks/... tools/perf/benchmarks/benchmark_smoke_unittest.py:135: method = SmokeTestGenerator(benchmark, num_pages=2) This fix is reasonable. But please check the cycle time of telemetry_perf_unittest in mac_chromium_rel_ng, win_chromium_rel_ng, win_chromium_x64_rel_ng, linux_android_rel_ng to make sure that they don't exceed 3 minutes.
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1152763002/#ps120001 (title: "Fix benchmark_smoke_unittest failure because tab_switching failed to switch tab if there's only one…")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152763002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by deanliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152763002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by deanliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1152763002/#ps140001 (title: "Fix broken unittest on Chrome")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152763002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by sullivan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152763002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by deanliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1152763002/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152763002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2eee3f91209b8ccf16e7f64c03d42fbafd724ebe Cr-Commit-Position: refs/heads/master@{#337191}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/1212213003/ by dsinclair@chromium.org. The reason for reverting is: Looks like most of the perf tree has gone red with the tab_switching tests.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
