Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2745103003:
Add new trivial sites memory benchmark. (Closed)
DescriptionAdd new trivial sites memory benchmark.
BUG=701184
Review-Url: https://codereview.chromium.org/2745103003
Cr-Commit-Position: refs/heads/master@{#456774}
Committed: https://chromium.googlesource.com/chromium/src/+/9d290469e44b03ec532900c2639e2b6127894c18
Patch Set 1 #Patch Set 2 : rename benchmark #
Total comments: 1
Patch Set 3 : Add owner. #
Messages
Total messages: 28 (9 generated)
erikchen@chromium.org changed reviewers: + nednguyen@google.com
nednguyen: Please review.
On 2017/03/14 00:32:09, erikchen wrote: > nednguyen: Please review. Are you in charge of all memory desktop benchmark? Besides this benchmark, are you planning to add any other memory desktop benchmark?
> Are you in charge of all memory desktop benchmark? Yes. https://bugs.chromium.org/p/chromium/issues/detail?id=686292. I will turn alerts on incrementally as the tests get denoised. >Besides this benchmark, are you planning to add any other memory desktop benchmark? Quite likely, but not in the near future. I would like to eventually reach a state where large memory regressions, when fixed, have coverage in the perf waterfall.
Description was changed from ========== Add new trivial sites memory benchmark. BUG=701184 ========== to ========== Add new trivial sites memory benchmark. BUG=701184 ==========
nednguyen@google.com changed reviewers: + perezju@chromium.org
On 2017/03/14 00:56:17, erikchen wrote: > > Are you in charge of all memory desktop benchmark? > Yes. https://bugs.chromium.org/p/chromium/issues/detail?id=686292. I will turn > alerts on incrementally as the tests get denoised. > > >Besides this benchmark, are you planning to add any other memory desktop > benchmark? > Quite likely, but not in the near future. I would like to eventually reach a > state where large memory regressions, when fixed, have coverage in the perf > waterfall. Let just name this benchmark "memory.desktop" then. I really want us to get away from the model of creating lots of different benchmark with similar purpose: https://docs.google.com/document/d/1t8JG5988sB8ncznEjjQNfurRl6V8B2xML-kX07cQX...
On 2017/03/14 00:58:46, nednguyen wrote: > On 2017/03/14 00:56:17, erikchen wrote: > > > Are you in charge of all memory desktop benchmark? > > Yes. https://bugs.chromium.org/p/chromium/issues/detail?id=686292. I will turn > > alerts on incrementally as the tests get denoised. > > > > >Besides this benchmark, are you planning to add any other memory desktop > > benchmark? > > Quite likely, but not in the near future. I would like to eventually reach a > > state where large memory regressions, when fixed, have coverage in the perf > > waterfall. > > Let just name this benchmark "memory.desktop" then. I really want us to get away > from the model of creating lots of different benchmark with similar purpose: > https://docs.google.com/document/d/1t8JG5988sB8ncznEjjQNfurRl6V8B2xML-kX07cQX... Done.
lgtm https://codereview.chromium.org/2745103003/diff/20001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory.py (right): https://codereview.chromium.org/2745103003/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory.py:55: @benchmark.Enabled('win') Can you also add: @benchmark.Owner(emails=['erikchen@chromium.org']) ?
On 2017/03/14 01:03:22, nednguyen wrote: > lgtm > > https://codereview.chromium.org/2745103003/diff/20001/tools/perf/benchmarks/m... > File tools/perf/benchmarks/memory.py (right): > > https://codereview.chromium.org/2745103003/diff/20001/tools/perf/benchmarks/m... > tools/perf/benchmarks/memory.py:55: @benchmark.Enabled('win') > Can you also add: > mailto:@benchmark.Owner(emails=[ ? Is there a typo? I don't see any references to this anywhere in tools/perf/benchmarks: https://cs.chromium.org/search/?q=owner+file:tools/perf/benchmarks&type=cs
On 2017/03/14 01:06:51, erikchen wrote: > On 2017/03/14 01:03:22, nednguyen wrote: > > lgtm > > > > > https://codereview.chromium.org/2745103003/diff/20001/tools/perf/benchmarks/m... > > File tools/perf/benchmarks/memory.py (right): > > > > > https://codereview.chromium.org/2745103003/diff/20001/tools/perf/benchmarks/m... > > tools/perf/benchmarks/memory.py:55: @benchmark.Enabled('win') > > Can you also add: > > mailto:@benchmark.Owner(emails=[ ? > > Is there a typo? I don't see any references to this anywhere in > tools/perf/benchmarks: > https://cs.chromium.org/search/?q=owner+file:tools/perf/benchmarks&type=cs Okay, I at least found where this is coming from: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... I can add it, but I'd appreciate it in the future if I wasn't the very first consumer of new features. [e.g. add this to some benchmarks with known owners].
On 2017/03/14 01:09:03, erikchen wrote: > On 2017/03/14 01:06:51, erikchen wrote: > > On 2017/03/14 01:03:22, nednguyen wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/2745103003/diff/20001/tools/perf/benchmarks/m... > > > File tools/perf/benchmarks/memory.py (right): > > > > > > > > > https://codereview.chromium.org/2745103003/diff/20001/tools/perf/benchmarks/m... > > > tools/perf/benchmarks/memory.py:55: @benchmark.Enabled('win') > > > Can you also add: > > > mailto:@benchmark.Owner(emails=[ ? > > > > Is there a typo? I don't see any references to this anywhere in > > tools/perf/benchmarks: > > https://cs.chromium.org/search/?q=owner+file:tools/perf/benchmarks&type=cs > > Okay, I at least found where this is coming from: > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > I can add it, but I'd appreciate it in the future if I wasn't the very first > consumer of new features. [e.g. add this to some benchmarks with known owners]. In fact, when I run: /Users/erikchen/projects/chromium/src/tools/perf/generate_perf_json.py, I get: AttributeError: 'module' object has no attribute 'Owner'
You're welcome to add decorator when it works, but do you mind if I land the CL in the meanwhile?
On 2017/03/14 01:10:16, erikchen wrote: > You're welcome to add decorator when it works, but do you mind if I land the CL > in the meanwhile? Ok Turn out the catapult roll is stuck, hence https://codereview.chromium.org/2743663004/ didn't go through :-(
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/03/14 02:25:48, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) We now have @Owner decorator everywhere in perf benchmark, can you please add that change back before landing the CL? THanks
On 2017/03/14 15:44:37, nednguyen wrote: > On 2017/03/14 02:25:48, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > We now have @Owner decorator everywhere in perf benchmark, can you please add > that change back before landing the CL? THanks Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2745103003/#ps40001 (title: "Add owner.")
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": 40001, "attempt_start_ts": 1489512752673270,
"parent_rev": "c87c5d279daf0657fcd941003d0d4b503410c725", "commit_rev":
"9d290469e44b03ec532900c2639e2b6127894c18"}
Message was sent while issue was closed.
Description was changed from ========== Add new trivial sites memory benchmark. BUG=701184 ========== to ========== Add new trivial sites memory benchmark. BUG=701184 Review-Url: https://codereview.chromium.org/2745103003 Cr-Commit-Position: refs/heads/master@{#456774} Committed: https://chromium.googlesource.com/chromium/src/+/9d290469e44b03ec532900c2639e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9d290469e44b03ec532900c2639e...
Message was sent while issue was closed.
On 2017/03/14 at 18:50:40, commit-bot wrote: > Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9d290469e44b03ec532900c2639e... Fyi: some telemetry_unittests failures on Windows coincide with this CL, might be related? https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...
Message was sent while issue was closed.
On 2017/03/15 01:41:52, qyearsley wrote: > On 2017/03/14 at 18:50:40, commit-bot wrote: > > Committed patchset #3 (id:40001) as > https://chromium.googlesource.com/chromium/src/+/9d290469e44b03ec532900c2639e... > > Fyi: some telemetry_unittests failures on Windows coincide with this CL, might > be related? > https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2... I don't see how this CL could break those tests. Is that issue being tracked somewhere? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
