Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2713093004:
Enable the EQT metric for system health benchmarks. (Closed)
DescriptionEnable the EQT metric for system health benchmarks.
BUG=chromium:625701
Review-Url: https://codereview.chromium.org/2713093004
Cr-Commit-Position: refs/heads/master@{#453120}
Committed: https://chromium.googlesource.com/chromium/src/+/00109a3ffd77b5ee226bf6584a18f16ba85ce5c2
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix TODO #Messages
Total messages: 18 (8 generated)
ulan@chromium.org changed reviewers: + nednguyen@google.com, tdresser@chromium.org
PTAL. This patch should be landed after https://codereview.chromium.org/2713043003/ rolls in chromium. Offtopic: Ned, I noticed that system health benchmarks depend on the page cycler benchmark to setup the categories for the loading metric. I think we can factor out (metric -> category dependency) into separate module and use it in all benchmarks in generic way. wdyt? If it sounds good, I can implement it in follow-up CL.
On 2017/02/24 15:57:52, ulan wrote: > PTAL. > > This patch should be landed after https://codereview.chromium.org/2713043003/ > rolls in chromium. LGTM > > Offtopic: Ned, I noticed that system health benchmarks depend on the page cycler > benchmark to setup the categories for the loading metric. I think we can factor > out (metric -> category dependency) into separate module and use it in all > benchmarks in generic way. wdyt? If it sounds good, I can implement it in > follow-up CL. This would be awesome!
Description was changed from ========== Enabale the EQT metric for system health benchmarks. BUG=chromium:625701 ========== to ========== Enabale the EQT metric for system health benchmarks. BUG=chromium:625701 ==========
nednguyen@google.com changed reviewers: + charliea@chromium.org
On 2017/02/24 16:08:15, nednguyen wrote: > On 2017/02/24 15:57:52, ulan wrote: > > PTAL. > > > > This patch should be landed after https://codereview.chromium.org/2713043003/ > > rolls in chromium. > > LGTM > > > > Offtopic: Ned, I noticed that system health benchmarks depend on the page > cycler > > benchmark to setup the categories for the loading metric. I think we can > factor > > out (metric -> category dependency) into separate module and use it in all > > benchmarks in generic way. wdyt? If it sounds good, I can implement it in > > follow-up CL. > > This would be awesome! +Charlie: FYI
Description was changed from ========== Enabale the EQT metric for system health benchmarks. BUG=chromium:625701 ========== to ========== Enable the EQT metric for system health benchmarks. BUG=chromium:625701 ==========
Thanks!
https://codereview.chromium.org/2713093004/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2713093004/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health.py:47: # TODO(all): Remove dependency on page_cycler_v2. nit: I'd just put your name in the TODO. I've always been told that the person whose name is in the TODO isn't the one responsible for actually doing the thing, just a good resource to contact about how to go about doing the thing.
lgtm w/ above comment
Thank you, Charlie! Do you have few minutes to look at a tiny catapult CL that is blocking this CL? ( https://codereview.chromium.org/2713043003/) https://codereview.chromium.org/2713093004/diff/1/tools/perf/benchmarks/syste... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2713093004/diff/1/tools/perf/benchmarks/syste... tools/perf/benchmarks/system_health.py:47: # TODO(all): Remove dependency on page_cycler_v2. On 2017/02/24 16:22:35, charliea wrote: > nit: I'd just put your name in the TODO. I've always been told that the person > whose name is in the TODO isn't the one responsible for actually doing the > thing, just a good resource to contact about how to go about doing the thing. Done.
LGTM, thanks!
The CQ bit was checked by ulan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2713093004/#ps20001 (title: "Fix TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488122307130750,
"parent_rev": "ddb49ff47dc851ae174a2224e1b43d42872f508b", "commit_rev":
"00109a3ffd77b5ee226bf6584a18f16ba85ce5c2"}
Message was sent while issue was closed.
Description was changed from ========== Enable the EQT metric for system health benchmarks. BUG=chromium:625701 ========== to ========== Enable the EQT metric for system health benchmarks. BUG=chromium:625701 Review-Url: https://codereview.chromium.org/2713093004 Cr-Commit-Position: refs/heads/master@{#453120} Committed: https://chromium.googlesource.com/chromium/src/+/00109a3ffd77b5ee226bf6584a18... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/00109a3ffd77b5ee226bf6584a18... |
