Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
789363002:
TimelineBasedMeasurement(object) instead of TimelineBasedMeasurement(page_test.PageTest) (Closed)
DescriptionTimelineBasedMeasurement(object) instead of TimelineBasedMeasurement(page_test.PageTest)
BUG=439544
Committed: https://crrev.com/5eea335f462d8e8ae82dae916e426a63b070cb48
Cr-Commit-Position: refs/heads/master@{#310240}
Patch Set 1 #Patch Set 2 : Rename again. Add synthetic delays back. Some doc strings. #
Total comments: 40
Patch Set 3 : Update docs. Add PageTest guards. #
Total comments: 8
Patch Set 4 : Address review comments. Unit tests in the works. #Patch Set 5 : Rebase #
Total comments: 26
Patch Set 6 : Add benchmark unit tests. #Patch Set 7 : Add user_story_runner.Run test for TBM (ensures no PageTest calls made). #Patch Set 8 : Drop absolute import change (put in another CL if needed). #Patch Set 9 : More review comments. #Patch Set 10 : Address outstanding review comments. #Patch Set 11 : Add TODOs for cleaning-up PageTest-specific code. #Patch Set 12 : Fix v8 benchmark. #Patch Set 13 : Skip TBM benchmarks for measurement_smoke_test. #Patch Set 14 : Fix lint error #Patch Set 15 : Lint fix the correct way. #Patch Set 16 : Rebase. #Messages
Total messages: 61 (12 generated)
Rename again. Add synthetic delays back. Some doc strings.
slamm@chromium.org changed reviewers: + chrishenry@google.com, nednguyen@google.com
https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:44: A test packages a PageTest and a PageSet together. Can we have a quick instructions on using this class btw? We'd want to reflect the new reality (TBM is default if page_test is not specified and CreatePageTest is not overridden). https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:182: return timeline_based_measurement.Options() pydoc please. Make sure to mention that one should only override one of these methods: CreatePageTest and CreateTBMOptions. Along with details as to when to override which. Perhaps update the class doc as well. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:188: Override to generate a custom page test. Update the doc to reflect above comment. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:191: return self.test() Is there a way to check here that CreateTBMOptions is not overridden if we're using this code path? https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:196: return timeline_based_measurement.TimelineBasedPageTest(tbm) FYI, the creation of PageTest probably should eventually belong to SharedPageState. This is fine for this patch, but it should not be needed except for SharedPageState. In fact, this causes as to pass a PageTest instance to SharedAppState, wouldn't that break due to the assertion there? Did you test our own android app benchmark against this change (probably want to update the assertion for now)? https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:117: This is typically created and returned by Nit: delete "typically". https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:121: def __init__(self, overhead_level=NO_OVERHEAD_LEVEL): Doc what overhead level means. We should also add that this is a temporary solution we added for v8 benchmark and may be removed in the future. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:154: app: an app.App subclass instance. I wonder whether we should just be passing tracing_controller directly? https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:192: def StopTracing(self, app): Same: should this be passing tracing_controller? https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:216: self._measurement.StopTracing(tab.browser) I guess this is one case where, if StartTracing can add some sort of cleanup routine to telemetry, we would have simpler code.
nednguyen@google.com changed reviewers: + ernstm@chromium.org
You may want to make sure that you don't break v8 benchmark. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:196: return timeline_based_measurement.TimelineBasedPageTest(tbm) On 2014/12/11 01:44:03, chrishenry wrote: > FYI, the creation of PageTest probably should eventually belong to > SharedPageState. This is fine for this patch, but it should not be needed except > for SharedPageState. In fact, this causes as to pass a PageTest instance to > SharedAppState, wouldn't that break due to the assertion there? Did you test our > own android app benchmark against this change (probably want to update the > assertion for now)? I don't understand this. why do we return TimelineBasedPageTest instance here instead of TimelineBasedMeasurement instance? https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:121: def __init__(self, overhead_level=NO_OVERHEAD_LEVEL): On 2014/12/11 01:44:03, chrishenry wrote: > Doc what overhead level means. We should also add that this is a temporary > solution we added for v8 benchmark and may be removed in the future. the V8 Overhead level is the only temporary solution. I think we still have no-overhead, minimum-overhead & debug-overhead level.
https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:199: def __init__(self, tbm): The constructor of this should take Options instead of tbm.
On 2014/12/11 16:42:20, nednguyen wrote: > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): > > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:199: def > __init__(self, tbm): > The constructor of this should take Options instead of tbm. Hm, I don't think this is the case. My thought is to eventually pass TBM to SharedPageState, and SPS will have to create a TBPageTest out of TBM. So TBPageTest has to take a TBM. Think of it as a wrapper that convert a given TBM to a PageTest.
https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:196: return timeline_based_measurement.TimelineBasedPageTest(tbm) On 2014/12/11 02:54:33, nednguyen wrote: > On 2014/12/11 01:44:03, chrishenry wrote: > > FYI, the creation of PageTest probably should eventually belong to > > SharedPageState. This is fine for this patch, but it should not be needed > except > > for SharedPageState. In fact, this causes as to pass a PageTest instance to > > SharedAppState, wouldn't that break due to the assertion there? Did you test > our > > own android app benchmark against this change (probably want to update the > > assertion for now)? > > I don't understand this. why do we return TimelineBasedPageTest instance here > instead of TimelineBasedMeasurement instance? I think for now it has to return TBPageTest, since we need PageTest in user_story_runner. The alternative would be to guard user_story_runner usage of PageTest with isinstance call, but that also seems fairly hacky. Any other ideas? Eventually, once PageTest is confined to SharedPageState only, we can return TBM here, pass it to user_story_runner and then SharedPageState, and then have SPS construct TBPT instead of here. But we are not nearly close to that. For now, a good enough fix may be to update SharedAppState to check for isinstance TBPT instead of TBM. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:121: def __init__(self, overhead_level=NO_OVERHEAD_LEVEL): On 2014/12/11 02:54:33, nednguyen wrote: > On 2014/12/11 01:44:03, chrishenry wrote: > > Doc what overhead level means. We should also add that this is a temporary > > solution we added for v8 benchmark and may be removed in the future. > > the V8 Overhead level is the only temporary solution. I think we still have > no-overhead, minimum-overhead & debug-overhead level. Do we want to allow benchmark to override overhead level? https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:154: app: an app.App subclass instance. On 2014/12/11 01:44:03, chrishenry wrote: > I wonder whether we should just be passing tracing_controller directly? +1
On 2014/12/11 17:48:56, chrishenry wrote: > On 2014/12/11 16:42:20, nednguyen wrote: > > > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): > > > > > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:199: def > > __init__(self, tbm): > > The constructor of this should take Options instead of tbm. > > Hm, I don't think this is the case. My thought is to eventually pass TBM to > SharedPageState, and SPS will have to create a TBPageTest out of TBM. So > TBPageTest has to take a TBM. Think of it as a wrapper that convert a given TBM > to a PageTest. I see.
https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:196: return timeline_based_measurement.TimelineBasedPageTest(tbm) On 2014/12/11 17:52:51, chrishenry wrote: > On 2014/12/11 02:54:33, nednguyen wrote: > > On 2014/12/11 01:44:03, chrishenry wrote: > > > FYI, the creation of PageTest probably should eventually belong to > > > SharedPageState. This is fine for this patch, but it should not be needed > > except > > > for SharedPageState. In fact, this causes as to pass a PageTest instance to > > > SharedAppState, wouldn't that break due to the assertion there? Did you test > > our > > > own android app benchmark against this change (probably want to update the > > > assertion for now)? > > > > I don't understand this. why do we return TimelineBasedPageTest instance here > > instead of TimelineBasedMeasurement instance? > > I think for now it has to return TBPageTest, since we need PageTest in > user_story_runner. The alternative would be to guard user_story_runner usage of > PageTest with isinstance call, but that also seems fairly hacky. Any other > ideas? > > Eventually, once PageTest is confined to SharedPageState only, we can return TBM > here, pass it to user_story_runner and then SharedPageState, and then have SPS > construct TBPT instead of here. But we are not nearly close to that. What is missing? I thought in our current state, most of the calls to page_test are delegated to shared_user_story_state except for the RequestExit()... that slamm@ will delete? > > For now, a good enough fix may be to update SharedAppState to check for > isinstance TBPT instead of TBM.
https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:196: return timeline_based_measurement.TimelineBasedPageTest(tbm) On 2014/12/11 17:58:43, nednguyen wrote: > On 2014/12/11 17:52:51, chrishenry wrote: > > On 2014/12/11 02:54:33, nednguyen wrote: > > > On 2014/12/11 01:44:03, chrishenry wrote: > > > > FYI, the creation of PageTest probably should eventually belong to > > > > SharedPageState. This is fine for this patch, but it should not be needed > > > except > > > > for SharedPageState. In fact, this causes as to pass a PageTest instance > to > > > > SharedAppState, wouldn't that break due to the assertion there? Did you > test > > > our > > > > own android app benchmark against this change (probably want to update the > > > > assertion for now)? > > > > > > I don't understand this. why do we return TimelineBasedPageTest instance > here > > > instead of TimelineBasedMeasurement instance? > > > > I think for now it has to return TBPageTest, since we need PageTest in > > user_story_runner. The alternative would be to guard user_story_runner usage > of > > PageTest with isinstance call, but that also seems fairly hacky. Any other > > ideas? > > > > Eventually, once PageTest is confined to SharedPageState only, we can return > TBM > > here, pass it to user_story_runner and then SharedPageState, and then have SPS > > construct TBPT instead of here. But we are not nearly close to that. > What is missing? I thought in our current state, most of the calls to page_test > are delegated to shared_user_story_state except for the RequestExit()... that > slamm@ will delete? > > > > > For now, a good enough fix may be to update SharedAppState to check for > > isinstance TBPT instead of TBM. I updated the bug thread: https://code.google.com/p/chromium/issues/detail?id=440101#c8
https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:196: return timeline_based_measurement.TimelineBasedPageTest(tbm) On 2014/12/11 18:03:20, chrishenry wrote: > On 2014/12/11 17:58:43, nednguyen wrote: > > On 2014/12/11 17:52:51, chrishenry wrote: > > > On 2014/12/11 02:54:33, nednguyen wrote: > > > > On 2014/12/11 01:44:03, chrishenry wrote: > > > > > FYI, the creation of PageTest probably should eventually belong to > > > > > SharedPageState. This is fine for this patch, but it should not be > needed > > > > except > > > > > for SharedPageState. In fact, this causes as to pass a PageTest instance > > to > > > > > SharedAppState, wouldn't that break due to the assertion there? Did you > > test > > > > our > > > > > own android app benchmark against this change (probably want to update > the > > > > > assertion for now)? > > > > > > > > I don't understand this. why do we return TimelineBasedPageTest instance > > here > > > > instead of TimelineBasedMeasurement instance? > > > > > > I think for now it has to return TBPageTest, since we need PageTest in > > > user_story_runner. The alternative would be to guard user_story_runner usage > > of > > > PageTest with isinstance call, but that also seems fairly hacky. Any other > > > ideas? > > > > > > Eventually, once PageTest is confined to SharedPageState only, we can return > > TBM > > > here, pass it to user_story_runner and then SharedPageState, and then have > SPS > > > construct TBPT instead of here. But we are not nearly close to that. > > What is missing? I thought in our current state, most of the calls to > page_test > > are delegated to shared_user_story_state except for the RequestExit()... that > > slamm@ will delete? > > > > > > > > For now, a good enough fix may be to update SharedAppState to check for > > > isinstance TBPT instead of TBM. > > I updated the bug thread: > https://code.google.com/p/chromium/issues/detail?id=440101#c8 Ok, seems like a lot to be fixed. How does SharedAppState convert TBPT back to TBM? https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:121: def __init__(self, overhead_level=NO_OVERHEAD_LEVEL): On 2014/12/11 17:52:52, chrishenry wrote: > On 2014/12/11 02:54:33, nednguyen wrote: > > On 2014/12/11 01:44:03, chrishenry wrote: > > > Doc what overhead level means. We should also add that this is a temporary > > > solution we added for v8 benchmark and may be removed in the future. > > > > the V8 Overhead level is the only temporary solution. I think we still have > > no-overhead, minimum-overhead & debug-overhead level. > > Do we want to allow benchmark to override overhead level? We do. A lot of time, minimal trace is not descriptive enough and people will need to override the overhead level to debug. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:150: def StartTracing(self, app, synthetic_delay_categories=None): This synetheic_delay_categories seems like it should belong to Options. It's fine to address this in other patches.
https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:196: return timeline_based_measurement.TimelineBasedPageTest(tbm) On 2014/12/11 18:18:16, nednguyen wrote: > On 2014/12/11 18:03:20, chrishenry wrote: > > On 2014/12/11 17:58:43, nednguyen wrote: > > > On 2014/12/11 17:52:51, chrishenry wrote: > > > > On 2014/12/11 02:54:33, nednguyen wrote: > > > > > On 2014/12/11 01:44:03, chrishenry wrote: > > > > > > FYI, the creation of PageTest probably should eventually belong to > > > > > > SharedPageState. This is fine for this patch, but it should not be > > needed > > > > > except > > > > > > for SharedPageState. In fact, this causes as to pass a PageTest > instance > > > to > > > > > > SharedAppState, wouldn't that break due to the assertion there? Did > you > > > test > > > > > our > > > > > > own android app benchmark against this change (probably want to update > > the > > > > > > assertion for now)? > > > > > > > > > > I don't understand this. why do we return TimelineBasedPageTest instance > > > here > > > > > instead of TimelineBasedMeasurement instance? > > > > > > > > I think for now it has to return TBPageTest, since we need PageTest in > > > > user_story_runner. The alternative would be to guard user_story_runner > usage > > > of > > > > PageTest with isinstance call, but that also seems fairly hacky. Any other > > > > ideas? > > > > > > > > Eventually, once PageTest is confined to SharedPageState only, we can > return > > > TBM > > > > here, pass it to user_story_runner and then SharedPageState, and then have > > SPS > > > > construct TBPT instead of here. But we are not nearly close to that. > > > What is missing? I thought in our current state, most of the calls to > > page_test > > > are delegated to shared_user_story_state except for the RequestExit()... > that > > > slamm@ will delete? > > > > > > > > > > > For now, a good enough fix may be to update SharedAppState to check for > > > > isinstance TBPT instead of TBM. > > > > I updated the bug thread: > > https://code.google.com/p/chromium/issues/detail?id=440101#c8 > > Ok, seems like a lot to be fixed. How does SharedAppState convert TBPT back to > TBM? We can expose measurement property in TBPT that return the original TBM (this is actually in this patch, but it's unused). https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:121: def __init__(self, overhead_level=NO_OVERHEAD_LEVEL): On 2014/12/11 18:18:16, nednguyen wrote: > On 2014/12/11 17:52:52, chrishenry wrote: > > On 2014/12/11 02:54:33, nednguyen wrote: > > > On 2014/12/11 01:44:03, chrishenry wrote: > > > > Doc what overhead level means. We should also add that this is a temporary > > > > solution we added for v8 benchmark and may be removed in the future. > > > > > > the V8 Overhead level is the only temporary solution. I think we still have > > > no-overhead, minimum-overhead & debug-overhead level. > > > > Do we want to allow benchmark to override overhead level? > > We do. A lot of time, minimal trace is not descriptive enough and people will > need to override the overhead level to debug. This is done via command-line tho right? Benchmark should not increase overhead level. I thought we agreed that we should expose option that enables optional metrics, instead of tweaking underlying tracing options. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:154: app: an app.App subclass instance. On 2014/12/11 17:52:52, chrishenry wrote: > On 2014/12/11 01:44:03, chrishenry wrote: > > I wonder whether we should just be passing tracing_controller directly? > > +1 (Just in case you're confused, I somehow thought Ned put up this comment. Turns out I'm +1'ing my own comment.)
On 2014/12/11 18:26:42, chrishenry wrote: > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/benchmark.py (right): > > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/benchmark.py:196: return > timeline_based_measurement.TimelineBasedPageTest(tbm) > On 2014/12/11 18:18:16, nednguyen wrote: > > On 2014/12/11 18:03:20, chrishenry wrote: > > > On 2014/12/11 17:58:43, nednguyen wrote: > > > > On 2014/12/11 17:52:51, chrishenry wrote: > > > > > On 2014/12/11 02:54:33, nednguyen wrote: > > > > > > On 2014/12/11 01:44:03, chrishenry wrote: > > > > > > > FYI, the creation of PageTest probably should eventually belong to > > > > > > > SharedPageState. This is fine for this patch, but it should not be > > > needed > > > > > > except > > > > > > > for SharedPageState. In fact, this causes as to pass a PageTest > > instance > > > > to > > > > > > > SharedAppState, wouldn't that break due to the assertion there? Did > > you > > > > test > > > > > > our > > > > > > > own android app benchmark against this change (probably want to > update > > > the > > > > > > > assertion for now)? > > > > > > > > > > > > I don't understand this. why do we return TimelineBasedPageTest > instance > > > > here > > > > > > instead of TimelineBasedMeasurement instance? > > > > > > > > > > I think for now it has to return TBPageTest, since we need PageTest in > > > > > user_story_runner. The alternative would be to guard user_story_runner > > usage > > > > of > > > > > PageTest with isinstance call, but that also seems fairly hacky. Any > other > > > > > ideas? > > > > > > > > > > Eventually, once PageTest is confined to SharedPageState only, we can > > return > > > > TBM > > > > > here, pass it to user_story_runner and then SharedPageState, and then > have > > > SPS > > > > > construct TBPT instead of here. But we are not nearly close to that. > > > > What is missing? I thought in our current state, most of the calls to > > > page_test > > > > are delegated to shared_user_story_state except for the RequestExit()... > > that > > > > slamm@ will delete? > > > > > > > > > > > > > > For now, a good enough fix may be to update SharedAppState to check for > > > > > isinstance TBPT instead of TBM. > > > > > > I updated the bug thread: > > > https://code.google.com/p/chromium/issues/detail?id=440101#c8 > > > > Ok, seems like a lot to be fixed. How does SharedAppState convert TBPT back to > > TBM? > > We can expose measurement property in TBPT that return the original TBM (this is > actually in this patch, but it's unused). > > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): > > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:121: def > __init__(self, overhead_level=NO_OVERHEAD_LEVEL): > On 2014/12/11 18:18:16, nednguyen wrote: > > On 2014/12/11 17:52:52, chrishenry wrote: > > > On 2014/12/11 02:54:33, nednguyen wrote: > > > > On 2014/12/11 01:44:03, chrishenry wrote: > > > > > Doc what overhead level means. We should also add that this is a > temporary > > > > > solution we added for v8 benchmark and may be removed in the future. > > > > > > > > the V8 Overhead level is the only temporary solution. I think we still > have > > > > no-overhead, minimum-overhead & debug-overhead level. > > > > > > Do we want to allow benchmark to override overhead level? > > > > We do. A lot of time, minimal trace is not descriptive enough and people will > > need to override the overhead level to debug. > > This is done via command-line tho right? Benchmark should not increase overhead > level. I thought we agreed that we should expose option that enables optional > metrics, instead of tweaking underlying tracing options. I don't recall that. I think what we agree on are: 1) All the useful metrics should be available with minimal overhead level. 2) The only exception is video based metrics, for now. 3) If people want to get more info for easier debugging, they should explicitly specify the overhead level they want to use. More overhead is unavoidable in these cases, and users should be informed of the trade-off. Specifying the overhead through either commandline or option is fine to me. > > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:154: app: an > app.App subclass instance. > On 2014/12/11 17:52:52, chrishenry wrote: > > On 2014/12/11 01:44:03, chrishenry wrote: > > > I wonder whether we should just be passing tracing_controller directly? > > > > +1 > > (Just in case you're confused, I somehow thought Ned put up this comment. Turns > out I'm +1'ing my own comment.)
On 2014/12/11 18:41:52, nednguyen wrote: > On 2014/12/11 18:26:42, chrishenry wrote: > > > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/benchmark.py (right): > > > > > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/benchmark.py:196: return > > timeline_based_measurement.TimelineBasedPageTest(tbm) > > On 2014/12/11 18:18:16, nednguyen wrote: > > > On 2014/12/11 18:03:20, chrishenry wrote: > > > > On 2014/12/11 17:58:43, nednguyen wrote: > > > > > On 2014/12/11 17:52:51, chrishenry wrote: > > > > > > On 2014/12/11 02:54:33, nednguyen wrote: > > > > > > > On 2014/12/11 01:44:03, chrishenry wrote: > > > > > > > > FYI, the creation of PageTest probably should eventually belong to > > > > > > > > SharedPageState. This is fine for this patch, but it should not be > > > > needed > > > > > > > except > > > > > > > > for SharedPageState. In fact, this causes as to pass a PageTest > > > instance > > > > > to > > > > > > > > SharedAppState, wouldn't that break due to the assertion there? > Did > > > you > > > > > test > > > > > > > our > > > > > > > > own android app benchmark against this change (probably want to > > update > > > > the > > > > > > > > assertion for now)? > > > > > > > > > > > > > > I don't understand this. why do we return TimelineBasedPageTest > > instance > > > > > here > > > > > > > instead of TimelineBasedMeasurement instance? > > > > > > > > > > > > I think for now it has to return TBPageTest, since we need PageTest in > > > > > > user_story_runner. The alternative would be to guard user_story_runner > > > usage > > > > > of > > > > > > PageTest with isinstance call, but that also seems fairly hacky. Any > > other > > > > > > ideas? > > > > > > > > > > > > Eventually, once PageTest is confined to SharedPageState only, we can > > > return > > > > > TBM > > > > > > here, pass it to user_story_runner and then SharedPageState, and then > > have > > > > SPS > > > > > > construct TBPT instead of here. But we are not nearly close to that. > > > > > What is missing? I thought in our current state, most of the calls to > > > > page_test > > > > > are delegated to shared_user_story_state except for the RequestExit()... > > > that > > > > > slamm@ will delete? > > > > > > > > > > > > > > > > > For now, a good enough fix may be to update SharedAppState to check > for > > > > > > isinstance TBPT instead of TBM. > > > > > > > > I updated the bug thread: > > > > https://code.google.com/p/chromium/issues/detail?id=440101#c8 > > > > > > Ok, seems like a lot to be fixed. How does SharedAppState convert TBPT back > to > > > TBM? > > > > We can expose measurement property in TBPT that return the original TBM (this > is > > actually in this patch, but it's unused). > > > > > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): > > > > > https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:121: def > > __init__(self, overhead_level=NO_OVERHEAD_LEVEL): > > On 2014/12/11 18:18:16, nednguyen wrote: > > > On 2014/12/11 17:52:52, chrishenry wrote: > > > > On 2014/12/11 02:54:33, nednguyen wrote: > > > > > On 2014/12/11 01:44:03, chrishenry wrote: > > > > > > Doc what overhead level means. We should also add that this is a > > temporary > > > > > > solution we added for v8 benchmark and may be removed in the future. > > > > > > > > > > the V8 Overhead level is the only temporary solution. I think we still > > have > > > > > no-overhead, minimum-overhead & debug-overhead level. > > > > > > > > Do we want to allow benchmark to override overhead level? > > > > > > We do. A lot of time, minimal trace is not descriptive enough and people > will > > > need to override the overhead level to debug. > > > > This is done via command-line tho right? Benchmark should not increase > overhead > > level. I thought we agreed that we should expose option that enables optional > > metrics, instead of tweaking underlying tracing options. > > I don't recall that. I think what we agree on are: > 1) All the useful metrics should be available with minimal overhead level. > 2) The only exception is video based metrics, for now. > 3) If people want to get more info for easier debugging, they should explicitly > specify the overhead level they want to use. More overhead is unavoidable in > these cases, and users should be informed of the trade-off. > > Specifying the overhead through either commandline or option is fine to me. Let's wait for real use case. I'm fine with keeping existing status quo. I suspect there will be cases where it'll just be too expensive to add to cc,benchmark (e.g., a hypothetical speedindex based on frame viewer).
https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:44: A test packages a PageTest and a PageSet together. On 2014/12/11 01:44:03, chrishenry wrote: > Can we have a quick instructions on using this class btw? We'd want to reflect > the new reality (TBM is default if page_test is not specified and CreatePageTest > is not overridden). Done. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:182: return timeline_based_measurement.Options() On 2014/12/11 01:44:02, chrishenry wrote: > pydoc please. Make sure to mention that one should only override one of these > methods: CreatePageTest and CreateTBMOptions. Along with details as to when to > override which. Perhaps update the class doc as well. Done. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:188: Override to generate a custom page test. On 2014/12/11 01:44:03, chrishenry wrote: > Update the doc to reflect above comment. Done. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:191: return self.test() On 2014/12/11 01:44:03, chrishenry wrote: > Is there a way to check here that CreateTBMOptions is not overridden if we're > using this code path? Done. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:193: if self.test in (None, timeline_based_measurement.TimelineBasedMeasurement): Should I add "test = None" at the top of this class. Or, should I use a hasattr test here instead? https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:117: This is typically created and returned by On 2014/12/11 01:44:03, chrishenry wrote: > Nit: delete "typically". Done. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:121: def __init__(self, overhead_level=NO_OVERHEAD_LEVEL): On 2014/12/11 01:44:03, chrishenry wrote: > Doc what overhead level means. We should also add that this is a temporary > solution we added for v8 benchmark and may be removed in the future. Done. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:150: def StartTracing(self, app, synthetic_delay_categories=None): On 2014/12/11 18:18:16, nednguyen wrote: > This synetheic_delay_categories seems like it should belong to Options. It's > fine to address this in other patches. Added a TODO. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:154: app: an app.App subclass instance. On 2014/12/11 18:26:42, chrishenry wrote: > On 2014/12/11 17:52:52, chrishenry wrote: > > On 2014/12/11 01:44:03, chrishenry wrote: > > > I wonder whether we should just be passing tracing_controller directly? > > > > +1 > > (Just in case you're confused, I somehow thought Ned put up this comment. Turns > out I'm +1'ing my own comment.) How would it check IsChromeTracingSupported(app) in that case? https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:192: def StopTracing(self, app): On 2014/12/11 01:44:03, chrishenry wrote: > Same: should this be passing tracing_controller? Will make it match what we decide for StartTracing. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:199: def __init__(self, tbm): On 2014/12/11 16:42:20, nednguyen wrote: > The constructor of this should take Options instead of tbm. Why is that? (I am still trying to understand how these classes all relate to each other.) https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:216: self._measurement.StopTracing(tab.browser) On 2014/12/11 01:44:03, chrishenry wrote: > I guess this is one case where, if StartTracing can add some sort of cleanup > routine to telemetry, we would have simpler code. Acknowledged. https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:67: test = None Since we check if the test is None, should I add this. Or, should the code check hasattr(self, 'test')?
Do you want to add a test to user_story_runner_unittest to verify that user_story_runner can run TimelineBasedMeasurement(object)? Otherwise, I am not sure how to verify the the effect of this patch. https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:67: test = None On 2014/12/19 00:45:48, slamm wrote: > Since we check if the test is None, should I add this. > Or, should the code check hasattr(self, 'test')? I prefer None checking over these attributes magic. https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:160: def StartTracing(self, app, synthetic_delay_categories=None): s/StartTracing/WillRunTest also s/StopTracing/DidRunTest
Can you create benchmark_unittest to add coverage? Benchmark class is complicated enough to deserve unittesting. https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:253: self.test.__name__) Hmh, how does this work if test is subclass for page_test?
https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:154: app: an app.App subclass instance. On 2014/12/19 00:45:48, slamm wrote: > On 2014/12/11 18:26:42, chrishenry wrote: > > On 2014/12/11 17:52:52, chrishenry wrote: > > > On 2014/12/11 01:44:03, chrishenry wrote: > > > > I wonder whether we should just be passing tracing_controller directly? > > > > > > +1 > > > > (Just in case you're confused, I somehow thought Ned put up this comment. > Turns > > out I'm +1'ing my own comment.) > > How would it check IsChromeTracingSupported(app) in that case? Fine as follow up (or not at all). See comment below. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:160: if not app.platform.tracing_controller.IsChromeTracingSupported(app): I wonder why this doesn't just look like Start()/Stop() (which doesn't require the app) and implicitly check _AssertOneBrowserBackend and then browser_backend.tracing_supported). Fine as follow up. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:192: def StopTracing(self, app): On 2014/12/19 00:45:48, slamm wrote: > On 2014/12/11 01:44:03, chrishenry wrote: > > Same: should this be passing tracing_controller? > > Will make it match what we decide for StartTracing. Well, this one doesn't require the app instance. https://codereview.chromium.org/789363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:199: def __init__(self, tbm): On 2014/12/19 00:45:48, slamm wrote: > On 2014/12/11 16:42:20, nednguyen wrote: > > The constructor of this should take Options instead of tbm. > > Why is that? (I am still trying to understand how these classes all relate to > each other.) Ignore this comment. There's a discussion that went on on the review thread on this already. https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:67: test = None On 2014/12/19 01:16:21, nednguyen wrote: > On 2014/12/19 00:45:48, slamm wrote: > > Since we check if the test is None, should I add this. > > Or, should the code check hasattr(self, 'test')? > > I prefer None checking over these attributes magic. Why not test = TimelineBasedMeasurement?
Address review comments. Unit tests in the works.
Rebase
https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:59: Alternatively, a benchmark packages a PageTest and a PageSet together. PageSet is always packaged with a measurement. It's just that you can use TBM or PageTest. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:64: A TimelineBasedMeasurement will be used if Benchmark.test is None, or if It's not None. I think you just say that: Benchmark default to TBM unless you override the value of Benchmark.test or CreatePageTest method. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:67: Benchmark creators can also override CreatePageSet or CreateUserStory set. New benchmark should always override CreateUserStorySet (no space). https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:87: raise InvalidOverrideError( I'd just replace this with an assertion assert has_custom..., 'Can not override both CreatePageTest and CreateTi...' https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:241: Override this method to configure TimelineBasedMeasurement tests. s/tests/benchmark/ https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark_unittest.py (right): https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark_unittest.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. Please add more unit tests. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark_unittest.py:5: from __future__ import absolute_import Let's continue conversation about this on the other code review. I'm inclined to rely on gclient runhooks. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/shared_page_state.py (right): https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/shared_page_state.py:49: else test) nit: I find that it's a bit easier to read long conditionals like this if written like so: if ...: self:_test = ... else: self._test = ... https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner.py (right): https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:265: should_discard_run = ( Rather than adding should, adding current is much more useful (discard_current_run). https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:160: def WillRunTest(self, tracing_controller, synthetic_delay_categories=None): This is WillRunUserStory right? Not Test (test describes the entire benchmark)? https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:193: def Measure(self, app, renderer_thread_tab_id, results): Pass tracing_controller instead? https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:203: def DidRunTest(self, tracing_controller): DidRunUserStory.
https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark_unittest.py (right): https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark_unittest.py:5: from __future__ import absolute_import On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > Let's continue conversation about this on the other code review. I'm inclined to > rely on gclient runhooks. So do I. Let's fix real cause rather than just fixing the symptoms.
Add benchmark unit tests.
On 2014/12/20 02:26:04, slamm wrote: > Add benchmark unit tests. Please add user_story_runner_unittest to validate that this patch make TBM runnable with user_story_runner. Otherwise it's hard to tell what work left to do.
[Telemetry] Drop check for devtools support for record-as-much-as-possible (chrome >2118)
Patchset #7 (id:120001) has been deleted
Add user_story_runner.Run test for TBM (ensures no PageTest calls made).
More review comments.
Address outstanding review comments.
https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:67: test = None On 2014/12/19 01:25:03, chrishenry wrote: > On 2014/12/19 01:16:21, nednguyen wrote: > > On 2014/12/19 00:45:48, slamm wrote: > > > Since we check if the test is None, should I add this. > > > Or, should the code check hasattr(self, 'test')? > > > > I prefer None checking over these attributes magic. > > Why not test = TimelineBasedMeasurement? Done. https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:253: self.test.__name__) On 2014/12/19 01:24:43, nednguyen wrote: > Hmh, how does this work if test is subclass for page_test? Fixed with issubclass. https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:160: def StartTracing(self, app, synthetic_delay_categories=None): On 2014/12/19 01:16:21, nednguyen wrote: > s/StartTracing/WillRunTest > also > s/StopTracing/DidRunTest Done. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:59: Alternatively, a benchmark packages a PageTest and a PageSet together. On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > PageSet is always packaged with a measurement. It's just that you can use TBM or > PageTest. Done. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:64: A TimelineBasedMeasurement will be used if Benchmark.test is None, or if On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > It's not None. I think you just say that: Benchmark default to TBM unless you > override the value of Benchmark.test or CreatePageTest method. Done. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:67: Benchmark creators can also override CreatePageSet or CreateUserStory set. On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > New benchmark should always override CreateUserStorySet (no space). Done. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:87: raise InvalidOverrideError( On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > I'd just replace this with an assertion > assert has_custom..., 'Can not override both CreatePageTest and CreateTi...' Done. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:241: Override this method to configure TimelineBasedMeasurement tests. On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > s/tests/benchmark/ Done. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark_unittest.py (right): https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark_unittest.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > Please add more unit tests. Done. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark_unittest.py:5: from __future__ import absolute_import On 2014/12/20 01:22:18, nednguyen wrote: > On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > > Let's continue conversation about this on the other code review. I'm inclined > to > > rely on gclient runhooks. > > So do I. Let's fix real cause rather than just fixing the symptoms. If anyone else runs into the problem of having a stale "telemetry/unittest" directory, they can run "gclient sync" to resolve it. (I opened another CL, however, I closed it because it seems to be a non-issue. The buildbots fixed it by clobbering.) https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/shared_page_state.py (right): https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/shared_page_state.py:49: else test) On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > nit: I find that it's a bit easier to read long conditionals like this if > written like so: > if ...: > self:_test = ... > else: > self._test = ... Done. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner.py (right): https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:265: should_discard_run = ( On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > Rather than adding should, adding current is much more useful > (discard_current_run). Done. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:265: should_discard_run = ( On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > Rather than adding should, adding current is much more useful > (discard_current_run). Done. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:160: def WillRunTest(self, tracing_controller, synthetic_delay_categories=None): On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > This is WillRunUserStory right? Not Test (test describes the entire benchmark)? Done. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:193: def Measure(self, app, renderer_thread_tab_id, results): On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > Pass tracing_controller instead? Done. https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:203: def DidRunTest(self, tracing_controller): On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > DidRunUserStory. Done.
On 2015/01/02 23:47:21, slamm wrote: > https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/benchmark.py (right): > > https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/benchmark.py:67: test = None > On 2014/12/19 01:25:03, chrishenry wrote: > > On 2014/12/19 01:16:21, nednguyen wrote: > > > On 2014/12/19 00:45:48, slamm wrote: > > > > Since we check if the test is None, should I add this. > > > > Or, should the code check hasattr(self, 'test')? > > > > > > I prefer None checking over these attributes magic. > > > > Why not test = TimelineBasedMeasurement? > > Done. > > https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/benchmark.py:253: self.test.__name__) > On 2014/12/19 01:24:43, nednguyen wrote: > > Hmh, how does this work if test is subclass for page_test? > > Fixed with issubclass. > > https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): > > https://codereview.chromium.org/789363002/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:160: def > StartTracing(self, app, synthetic_delay_categories=None): > On 2014/12/19 01:16:21, nednguyen wrote: > > s/StartTracing/WillRunTest > > also > > s/StopTracing/DidRunTest > > Done. > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/benchmark.py (right): > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/benchmark.py:59: Alternatively, a benchmark packages a > PageTest and a PageSet together. > On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > > PageSet is always packaged with a measurement. It's just that you can use TBM > or > > PageTest. > > Done. > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/benchmark.py:64: A TimelineBasedMeasurement will be > used if Benchmark.test is None, or if > On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > > It's not None. I think you just say that: Benchmark default to TBM unless you > > override the value of Benchmark.test or CreatePageTest method. > > Done. > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/benchmark.py:67: Benchmark creators can also override > CreatePageSet or CreateUserStory set. > On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > > New benchmark should always override CreateUserStorySet (no space). > > Done. > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/benchmark.py:87: raise InvalidOverrideError( > On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > > I'd just replace this with an assertion > > assert has_custom..., 'Can not override both CreatePageTest and CreateTi...' > > Done. > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/benchmark.py:241: Override this method to configure > TimelineBasedMeasurement tests. > On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > > s/tests/benchmark/ > > Done. > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/benchmark_unittest.py (right): > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/benchmark_unittest.py:1: # Copyright 2014 The Chromium > Authors. All rights reserved. > On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > > Please add more unit tests. > > Done. > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/benchmark_unittest.py:5: from __future__ import > absolute_import > On 2014/12/20 01:22:18, nednguyen wrote: > > On 2014/12/20 01:17:34, chrishenry (OOO until Jan 5) wrote: > > > Let's continue conversation about this on the other code review. I'm > inclined > > to > > > rely on gclient runhooks. > > > > So do I. Let's fix real cause rather than just fixing the symptoms. > > If anyone else runs into the problem of having a stale "telemetry/unittest" > directory, they can run "gclient sync" to resolve it. > (I opened another CL, however, I closed it because it seems to be a non-issue. > The buildbots fixed it by clobbering.) > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/page/shared_page_state.py (right): > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/page/shared_page_state.py:49: else test) > On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > > nit: I find that it's a bit easier to read long conditionals like this if > > written like so: > > if ...: > > self:_test = ... > > else: > > self._test = ... > > Done. > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/user_story/user_story_runner.py (right): > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/user_story/user_story_runner.py:265: > should_discard_run = ( > On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > > Rather than adding should, adding current is much more useful > > (discard_current_run). > > Done. > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/user_story/user_story_runner.py:265: > should_discard_run = ( > On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > > Rather than adding should, adding current is much more useful > > (discard_current_run). > > Done. > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:160: def > WillRunTest(self, tracing_controller, synthetic_delay_categories=None): > On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > > This is WillRunUserStory right? Not Test (test describes the entire > benchmark)? > > Done. > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:193: def > Measure(self, app, renderer_thread_tab_id, results): > On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > > Pass tracing_controller instead? > > Done. > > https://codereview.chromium.org/789363002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:203: def > DidRunTest(self, tracing_controller): > On 2014/12/20 01:17:35, chrishenry (OOO until Jan 5) wrote: > > DidRunUserStory. > > Done. Please add TODO comment to remove all the special casing in user_story_runner. Do we have bugs filed for removing these hooks yet?
lgtm
Add TODOs for cleaning-up PageTest-specific code.
Fix v8 benchmark.
lgtm
Skip TBM benchmarks for measurement_smoke_test.
The CQ bit was checked by slamm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/789363002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Fix lint error
The CQ bit was checked by slamm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/789363002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by slamm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/789363002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Lint fix the correct way.
The CQ bit was checked by slamm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/789363002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Rebase.
The CQ bit was checked by slamm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/789363002/320001
Message was sent while issue was closed.
Committed patchset #16 (id:320001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/5eea335f462d8e8ae82dae916e426a63b070cb48 Cr-Commit-Position: refs/heads/master@{#310240} |
