Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11428107:
Telemetry: extends Platform abstraction. (Closed)
|
Created:
8 years ago by bulach Modified:
7 years, 9 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, pam+watch_chromium.org, telemetry+watch_chromium.org, jeremy Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTelemetry: extends Platform abstraction.
It allows Telemetry on android to:
- sets the correct cpu governor
- install thermal throttling monitor
BUG=
TEST=tools/telemetry/run_tests --browser=android-chrome ScrollingActionTest PageRunner
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184153
Patch Set 1 #
Total comments: 1
Patch Set 2 : Single platform #Patch Set 3 : Platform API #
Total comments: 21
Patch Set 4 : PlatformBackend #Patch Set 5 : Rebased #
Total comments: 22
Patch Set 6 : aberent/tonyg #
Total comments: 2
Patch Set 7 : __init__/close() #
Total comments: 1
Messages
Total messages: 31 (0 generated)
Why not just have browser_options.disable_power_throttling and have the android_browser_backend do the rest?
sorry, not sure I understand... :) there's no such a thing as "disable power throttling" unfortunately... the best we can do is detect it has happened during a test... afaict, android_browser_backend doesn't have the enter / exit scope necessary for that detection.
I think this exposes too much detail up to tests, so we have to come up with something that keeps the abstraction low. Maybe we shoudl ignore the "up throtle when finger-down" for now and focus on "up throttle whenever telemetry is running." Thats definitely doing (just adb shell echo performacne to /sys/kern/blah/governorblah) Then, maybe we can do the finger-down throttling using some userland-driven approach, e.g. have the smoothScrollBy implementation in chromeview.java do some ioctl that triggers the right throttling behavior. Or we delegate the entire smooth drag implementation to the platform, somehow. But basically, do the power stuff there within the smoohtscrolldown
I think tonyg can explain in more details.... :) there are two completely orthogonal things here: - cpu governor: this is how aggressive the kernel will detect "idle" / "in use" and speed up / slow down the cpu.. downstream, we set for "performance" for the whole test. this essentially avoids the "idle detection" and will the cpu running as fast as it can... in this patch, this is done via self._perf_tests_setup.SetUp()... - thermal: the kernel detect when a CPU is too hot, and then it slows it down until its below a threshold. there's absolutely nothing we can do for this besides detecting and warning... imho, I think these belong in the harness, not the app... the app itself won't have access to either of these, as it runs under an unprivileged user..
sure, thanks! I'll look their patches as well, thanks for the pointers!
gargh! :) message above and new reviewers added to the wrong patch... sorry! hartmanng / vollick, you're more than welcome to review anyways...
I think I get the scenario a bit better now. Thanks for sorting me out!
A few challenges --- page sets have pass or fail. It seems to me page_runner
should fail tests that thermally throttle, if they're marke as perf tests.
Right?
Assuming thats kosher to you, that implies to me that page runner should have an
arg that says that its a perf test or not, or we should make a perf_page_runner
subclass that does perf related checks during page cleanup.
I do get that the platform interface is technically separate from the browser
object. But, I think its probably ok to just have it always created as part of
the browser object, and then have more explicit methods for starting/stopping
full power mode (DisablePowerThrottling=), IsThermallyLimited, etc. E..g
browser.platform.disable_power_throttling = is_perf_test
if browser.platform.is_throttled:
results.add_failure(page, "throttled") # just skip the test if we discover
this before running?
Note, your names are probably better than mine. My brain is foggy. I think
platform_interface is a decent name for the class, but browser.platform probably
is fine too. Lots of """docsrings""" though in whatever you come up with, and
file bugs for CrOS and esktopsupport for equivalent.
In the medium term, we wan to to be able to run scrolling tests with power
managment **enabled**. This may be a specific subcase different from say a
scrolling test -- but the key is that when your finger is down on the page, the
jelly bean power governor actually boosts the power frequency on all cores,
irregardless of what the software governor says. To get the best results [in a
followup to this patch], we should to emulate that behavior --- otherwise, we're
measuring smoothness of the system when its going full tilt, which isn't always
appropriate. This is the piece that I was saying there's an android guy
experimenting with
But, this patch pair is a good first step. Its definitely better than what we've
got now.
https://codereview.chromium.org/11428107/diff/1/tools/telemetry/telemetry/and... File tools/telemetry/telemetry/android_browser_finder.py (right): https://codereview.chromium.org/11428107/diff/1/tools/telemetry/telemetry/and... tools/telemetry/telemetry/android_browser_finder.py:18: sys.path.append( please call through adb_commands by wrapping. That wrapping layer Its there so we dont muck with imports all the time as well as so we can gracefully handle lack of pylib during a bootstrap scenario.
On Tue, Dec 4, 2012 at 9:20 AM, <nduca@chromium.org> wrote: > > A few challenges --- page sets have pass or fail. It seems to me > page_runner > should fail tests that thermally throttle, if they're marke as perf tests. > Right? > > Actually probably not. The point of the performance tests is to determine whether the performance is at an acceptable level (possibly relative to a previous level), and as such they should pass if it is, and fail if it isn't. The current clank performance tests always pass, and we record the results on graph for later examination, but you can imagine us agreeing some criteria on which they should fail (e.g. performance worse than the previous release). If a test is thermally throttled it means that we didn't get a meaningful result, not that there is a problem with performance. Since we run each test multiple times in order to get a statistically meaningful result, it is quite likely that as we run the tests the device will warm up and will thermally throttle on some of the later runs. The clank tests deal with this by throwing away that result, and, after a delay to allow the device to cool down, rerunning the test run. They only abandon running a particular test if a test thermally throttles a large number (currently 5) of times in a row. Even if a particular test is abandoned it does not abandon the test run, since the results of the other tests may still be useful. At the moment even abandoning a test is not treated as a test failure, since it doesn't imply a performance problems, but maybe it should be. - Anthony
Thanks anthony for the helpful comments. A question: if each run of the perf test is throttled, does the value show up in a perf graph? Failed in this context means the data is put into a graph, thats' the basic question I'm trying to wrap my head around. I do like the idea of adding a feature to page_runner to note the failed tests and try them again at the end of the run.
If all runs are throttled then no result is recorded and nothing is shown perf graph for that particular test. On Tue, Dec 4, 2012 at 9:53 AM, <nduca@chromium.org> wrote: > Thanks anthony for the helpful comments. A question: if each run of the > perf > test is throttled, does the value show up in a perf graph? Failed in this > context means the data is put into a graph, thats' the basic question I'm > trying > to wrap my head around. > > I do like the idea of adding a feature to page_runner to note the failed > tests > and try them again at the end of the run. > > https://codereview.chromium.**org/11428107/<https://codereview.chromium.org/1... >
nat: as discussed, there's just one Platform abstraction now, please let me know if it smells any better :)
Getting there. Can it just be created and managed by browser? I dont see value in creating it separately. E.g. I'd like to see browser.platform Platform: def IsThermallyThrottled() def SetFullPerformanceModeEnabled(enabled) def IsRawDisplayFrameRateSupported() def GetRawDisplayFrameRate() (or jank) The enter/exit stuff can be ignored, and page_runner can be modified to do the right thing around SetFullPerformance and IsThermallyThrottled.
hmm, that's the problem :) all of these APIs require a "scope", i.e., enter in a clean state, then let the "thing" proceed, then tear down and detect what happened / restore the initial state. right now, all of this is hidden away as an implementation detail of these platforms / stubs. we could certainly expose Start / End methods for all of them, but the caller code would look a bit awkward.. as it is, the "with" clause clearly delineate the scope of them, and we don't need to care about "oh, is it ok to interleave StartFoo with StartBar / EndFoo / EndBar?".. (we've got a few of these cases downstream..). wdyt?
I say fix it with docs and a bit of defensive code, explained later. I think that in the context of the page runner, we have well defined ways of managing scope, so its okay. And we could always have the close() on the Platform class make sure that any changes made to the platform are turned off when it shuts down in case the user of the class was an idiot.
thanks nat! I think the API as you suggested is more verbose and has a greater surface area now, but I don't have a strong opinion either way... :) ptal and let me know if this is the way you want to go.. Thanks!
Sorry to be so insistent on more api surface. Brevity is not a goal here. Our goal is robustness and easy-to-grokness. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... File tools/telemetry/telemetry/multi_page_benchmark_runner.py (left): https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/multi_page_benchmark_runner.py:79: runner.Run(options, possible_browser, benchmark, results) pass in performance_test=True to runner or put on the options object https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page_runner.py (right): https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/page_runner.py:38: if self.browser.platform.CanMonitorThermalThrottling(): this check should be somewhere else--- probably after each page runs, you should check for thermal monitoring and as noted in the previous discussion, mark it as failing, no? https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/page_runner.py:40: self.browser.platform.SetFullPerformanceModeEnabled(False) this should be conditional on performance tests being true https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/page_runner.py:124: state.browser.platform.SetFullPerformanceModeEnabled(True) so should this https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/page_runner.py:124: state.browser.platform.SetFullPerformanceModeEnabled(True) why not inside _SetupBrowser? https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... File tools/telemetry/telemetry/platform.py (left): https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:5: class Platform(object): By subclassing the platform, you override the doc strings. We want the docstrings to be unchanged. Two options 1. put the platform-specifc implementations into a class called PlatformBackend that you construct Platform with, then each method here does self._backend.GetRawDisplayFrameRate() 2. construct Platform with a browser_backend pointer and instead call right to that self._backend.GetRawDisplayFrameRate() https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... File tools/telemetry/telemetry/platform.py (right): https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:16: def StartRawDisplayFrameRate(self, trace_tag): Not clear whether you need a stop. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:21: """Prints GL surface stats.""" Not clear from the API if this can be called multiple times after startRaw, or only once. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:25: """Platforms may tweak their CPU governor, system status, etc.""" You might want to improve this docstring. Something like Most platforms can operate in a battery saving mode. While good for battery life, this can cause confusing performance results. Turning full performance mode on disables these features, which is useful for performance testing. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:28: def CanMonitorThermalThrottling(self): Again, more docstring is good Some fan-less computers go into a reduced performance mode when their heat exceeds a certain threshold. Performance tests in particular should use this API to detect if this has happened and interpret results accordingly. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:36: def StopMonitoringThermalThrottling(self): Why start and stop? Why not just IsDeviceThermallyThrottled? You shouldn't log inside this code, you should return a bool. The page runner should handle the error.
thanks nat! to be clear: I'm not against more API surface, sorry if I came across that way... as mentioned before, I have no strong opinion on either design, so happy to go with this.. :) and thanks for sending the docstrings too, I really appreciate your effort to avoid roundtrips! all comments addressed, another look please? https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... File tools/telemetry/telemetry/multi_page_benchmark_runner.py (left): https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/multi_page_benchmark_runner.py:79: runner.Run(options, possible_browser, benchmark, results) On 2012/12/07 18:12:06, nduca wrote: > pass in performance_test=True to runner or put on the options object Done as an option https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page_runner.py (right): https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/page_runner.py:38: if self.browser.platform.CanMonitorThermalThrottling(): On 2012/12/07 18:12:06, nduca wrote: > this check should be somewhere else--- probably after each page runs, you should > check for thermal monitoring and as noted in the previous discussion, mark it as > failing, no? good point, done. raising an exception at the end of the test, we may want to revisit it later.. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/page_runner.py:40: self.browser.platform.SetFullPerformanceModeEnabled(False) On 2012/12/07 18:12:06, nduca wrote: > this should be conditional on performance tests being true Done. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/page_runner.py:124: state.browser.platform.SetFullPerformanceModeEnabled(True) On 2012/12/07 18:12:06, nduca wrote: > why not inside _SetupBrowser? Done. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... File tools/telemetry/telemetry/platform.py (left): https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:5: class Platform(object): On 2012/12/07 18:12:06, nduca wrote: > By subclassing the platform, you override the doc strings. We want the > docstrings to be unchanged. > > Two options > 1. put the platform-specifc implementations into a class called PlatformBackend > that you construct Platform with, then each method here does > self._backend.GetRawDisplayFrameRate() > 2. construct Platform with a browser_backend pointer and instead call right to > that > self._backend.GetRawDisplayFrameRate() ahn, thanks! I didn't know the the docstrings were overriden, I thought they were inherited.. done.. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... File tools/telemetry/telemetry/platform.py (right): https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:16: def StartRawDisplayFrameRate(self, trace_tag): On 2012/12/07 18:12:06, nduca wrote: > Not clear whether you need a stop. Done. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:21: """Prints GL surface stats.""" On 2012/12/07 18:12:06, nduca wrote: > Not clear from the API if this can be called multiple times after startRaw, or > only once. Done. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:25: """Platforms may tweak their CPU governor, system status, etc.""" On 2012/12/07 18:12:06, nduca wrote: > You might want to improve this docstring. Something like > > Most platforms can operate in a battery saving mode. While good for battery > life, this can cause confusing performance results. Turning full performance > mode on disables these features, which is useful for performance testing. Done. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:28: def CanMonitorThermalThrottling(self): On 2012/12/07 18:12:06, nduca wrote: > Again, more docstring is good > > Some fan-less computers go into a reduced performance mode when their heat > exceeds a certain threshold. Performance tests in particular should use this API > to detect if this has happened and interpret results accordingly. Done. https://codereview.chromium.org/11428107/diff/15001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:36: def StopMonitoringThermalThrottling(self): On 2012/12/07 18:12:06, nduca wrote: > Why start and stop? Why not just IsDeviceThermallyThrottled? You shouldn't log > inside this code, you should return a bool. The page runner should handle the > error. Done.
I just realized that this was still in my review list. Sorry I didn't review it earlier. I think your current code will miss thermal throttling that happens and recovers during a test, and you possibly need to reverse some of the changes you made between patch sets 3 and 4 to fix this (although I haven't reviewed patch set 3 in detail. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/android_platform_backend.py (right): https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/android_platform_backend.py:56: def IsThermallyThrottled(self): This asks whether the device is currently thermally throttled. This makes sense before you start a test, but what you really need to ask is whether the device was thermally throttled at any time during the test. If it is you should ignore the test result, give the device time to cool down, and then rerun the test. Since you presumably can't continuously run IsThermallyThrottled while the test is running you need to do this by looking for log messages over a known period. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page_runner.py (right): https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/page_runner.py:143: state.browser.platform.IsThermallyThrottled()): See my other comments on thermal throttling. This is not an adequate test since the device may have recovered before the test finishes (it doesn't take long to recover once it slows the clock). You need to (indirectly) call ThermalThrottle.HasBeenThrottled or similar. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/platform.py (right): https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:46: def IsThermallyThrottled(self): See comment in android_platform.py
A few quesitons/comments. Everything else looks good to me. Regarding Jeremy's use case about warm and cold startups, I think we should use the API as it exists today and modify it for that use case in a subsequent patch. https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... File tools/telemetry/telemetry/android_platform_backend.py (right): https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... tools/telemetry/telemetry/android_platform_backend.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. You can update to 2013 now. Boo for long reviews. https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... tools/telemetry/telemetry/android_platform_backend.py:14: from pylib import perf_tests_helper # pylint: disable=F0401 2 spaces between code and comments https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... tools/telemetry/telemetry/android_platform_backend.py:31: self._thermal_throttle = thermal_throttle.ThermalThrottle(self._adb) This uses adb as one arg and self._adb as the other. Please be consistent. https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... File tools/telemetry/telemetry/browser_options.py (right): https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... tools/telemetry/telemetry/browser_options.py:80: group.add_option('--performance-test', action='store_true', Why is this an option? Shouldn't we just do this by default? If we need a way to disable it for some use-case, then I'd recommend flipping this flag to default to true. https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... File tools/telemetry/telemetry/multi_page_benchmark_runner.py (right): https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... tools/telemetry/telemetry/multi_page_benchmark_runner.py:80: Revert this file. https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... File tools/telemetry/telemetry/page_runner.py (right): https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... tools/telemetry/telemetry/page_runner.py:145: 'performance tests.') I'm on the fence about whether this should really raise an exception or just print a warning. I guess the real question is how flaky this will make the bots. Perhaps we should only do one thing at a time. First, in this patch, disable frequency scaling and we can see how that affects noise on the bots. Then, in a subsequent patch, change the thermal throttling warning to an exception and we can monitor the bots closely to see if they become flaky. https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... tools/telemetry/telemetry/page_runner.py:234: state.browser.platform.SetFullPerformanceModeEnabled(True) I wonder if it would be more robust to call SetFullPerformanceModeEnable(True/False) from within the Browser class itself (in Create and Close). https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... File tools/telemetry/telemetry/possible_browser.py (right): https://chromiumcodereview.appspot.com/11428107/diff/21001/tools/telemetry/te... tools/telemetry/telemetry/possible_browser.py:5: Revert this file
thanks anthony, tony! all addressed, ptal. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/android_platform_backend.py (right): https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/android_platform_backend.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/21 18:07:08, tonyg wrote: > You can update to 2013 now. Boo for long reviews. ++year! :) https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/android_platform_backend.py:14: from pylib import perf_tests_helper # pylint: disable=F0401 On 2013/02/21 18:07:08, tonyg wrote: > 2 spaces between code and comments Done. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/android_platform_backend.py:31: self._thermal_throttle = thermal_throttle.ThermalThrottle(self._adb) On 2013/02/21 18:07:08, tonyg wrote: > This uses adb as one arg and self._adb as the other. Please be consistent. Done. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/android_platform_backend.py:56: def IsThermallyThrottled(self): On 2013/01/29 14:03:36, aberent wrote: > This asks whether the device is currently thermally throttled. This makes sense > before you start a test, but what you really need to ask is whether the device > was thermally throttled at any time during the test. If it is you should ignore > the test result, give the device time to cool down, and then rerun the test. > Since you presumably can't continuously run IsThermallyThrottled while the test > is running you need to do this by looking for log messages over a known period. good point! added a "HasBeenThrottled" and changed the checks.. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/browser_options.py (right): https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/browser_options.py:80: group.add_option('--performance-test', action='store_true', On 2013/02/21 18:07:08, tonyg wrote: > Why is this an option? Shouldn't we just do this by default? If we need a way to > disable it for some use-case, then I'd recommend flipping this flag to default > to true. good point, removed.. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/multi_page_benchmark_runner.py (right): https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/multi_page_benchmark_runner.py:80: On 2013/02/21 18:07:08, tonyg wrote: > Revert this file. Done. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page_runner.py (right): https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/page_runner.py:143: state.browser.platform.IsThermallyThrottled()): On 2013/01/29 14:03:36, aberent wrote: > See my other comments on thermal throttling. This is not an adequate test since > the device may have recovered before the test finishes (it doesn't take long to > recover once it slows the clock). You need to (indirectly) call > ThermalThrottle.HasBeenThrottled or similar. good point, added a check for "IsThrottled" at the beginning of the test, and then HasBeenThrottled to the end of it, please double check. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/page_runner.py:145: 'performance tests.') On 2013/02/21 18:07:08, tonyg wrote: > I'm on the fence about whether this should really raise an exception or just > print a warning. I guess the real question is how flaky this will make the bots. > > Perhaps we should only do one thing at a time. First, in this patch, disable > frequency scaling and we can see how that affects noise on the bots. Then, in a > subsequent patch, change the thermal throttling warning to an exception and we > can monitor the bots closely to see if they become flaky. agree. done as just messages for now. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/page_runner.py:234: state.browser.platform.SetFullPerformanceModeEnabled(True) On 2013/02/21 18:07:08, tonyg wrote: > I wonder if it would be more robust to call > SetFullPerformanceModeEnable(True/False) from within the Browser class itself > (in Create and Close). Done. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/platform.py (right): https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/platform.py:46: def IsThermallyThrottled(self): On 2013/01/29 14:03:36, aberent wrote: > See comment in android_platform.py Done. https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... File tools/telemetry/telemetry/possible_browser.py (right): https://codereview.chromium.org/11428107/diff/21001/tools/telemetry/telemetry... tools/telemetry/telemetry/possible_browser.py:5: On 2013/02/21 18:07:08, tonyg wrote: > Revert this file Done.
lgtm https://chromiumcodereview.appspot.com/11428107/diff/34008/tools/telemetry/te... File tools/telemetry/telemetry/core/browser.py (right): https://chromiumcodereview.appspot.com/11428107/diff/34008/tools/telemetry/te... tools/telemetry/telemetry/core/browser.py:93: self._platform.SetFullPerformanceModeEnabled(False) It feels really strange to have one in __enter__ and the other in Close. I recommend __enter__ and __exit__ or else __init__ and Close. Not sure which is more appropriate.
thanks tony! addressed, CQing.. https://codereview.chromium.org/11428107/diff/34008/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/browser.py (right): https://codereview.chromium.org/11428107/diff/34008/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser.py:93: self._platform.SetFullPerformanceModeEnabled(False) On 2013/02/22 17:19:09, tonyg wrote: > It feels really strange to have one in __enter__ and the other in Close. I > recommend __enter__ and __exit__ or else __init__ and Close. Not sure which is > more appropriate. yeah, it does look weird... I went for __init__ and Close() so enter/exit are unchanged..
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/11428107/39001
Message was sent while issue was closed.
Change committed as 184153
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/11428107/diff/39001/tools/telemetry/te... File tools/telemetry/telemetry/core/chrome/platform.py (right): https://chromiumcodereview.appspot.com/11428107/diff/39001/tools/telemetry/te... tools/telemetry/telemetry/core/chrome/platform.py:12: def __init__(self, platform_backend): This breaks PosssibleCrOSBrowser.Create at line 30: https://code.google.com/codesearch#OAMlx_jo-ck/src/tools/telemetry/telemetry/...
Message was sent while issue was closed.
Did we have unit tests that would have caught this regression?
Message was sent while issue was closed.
On 2013/02/25 21:22:54, nduca wrote: > Did we have unit tests that would have caught this regression? Yes, pretty much any cros-chrome unit test. They just weren't run.
Message was sent while issue was closed.
On 2013/02/25 21:27:23, achuith.bhandarkar wrote: > On 2013/02/25 21:22:54, nduca wrote: > > Did we have unit tests that would have caught this regression? > > Yes, pretty much any cros-chrome unit test. They just weren't run. https://codereview.chromium.org/12335073/
Message was sent while issue was closed.
ops, sorry about the breakage! thanks for the quick fix! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
