Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2257173003:
[tools/perf] Add CodeAndMetadata metrics to v8_browsing benchmarks and add Ignition variants (Closed)
|
Created:
4 years, 4 months ago by rmcilroy Modified:
4 years, 3 months ago Reviewers:
nduca, jochen (gone - plz use gerrit), nednguyen, petrcermak, Hannes Payer (out of office), ulan, sullivan CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tools/perf] Add CodeAndMetadata metrics to v8_browsing benchmarks and add Ignition variants
Adds CodeAndMetadata measurements to the v8_browsing telemetry benchmarks.
Modifes the memory dumper to dump these stats on light dumps, if requested.
Also adds an Ignition variant for v8_browsing benchmarks.
BUG=v8:4280, v8:5019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;
Committed: https://crrev.com/6b6b89ace1fded16dd2901e2de16b0346aeb3e5a
Cr-Commit-Position: refs/heads/master@{#414439}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address comments #
Messages
Total messages: 34 (16 generated)
Description was changed from ========== Add CodeAndMetadata metrics to v8_browse and add Ignition variants. Adds CodeAndMetadata measurements to the v8_browse telemetry benchmarks. Modifes the memory dumper to dump these stats on light dumps, if requested. Also adds an Ignition variant for v8_browse benchmarks. BUG=v8:4280,v8:5019 ========== to ========== Add CodeAndMetadata metrics to v8_browse and add Ignition variants. Adds CodeAndMetadata measurements to the v8_browse telemetry benchmarks. Modifes the memory dumper to dump these stats on light dumps, if requested. Also adds an Ignition variant for v8_browse benchmarks. BUG=v8:4280,v8:5019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
rmcilroy@chromium.org changed reviewers: + hpayer@chromium.org, petrcermak@chromium.org, ulan@chromium.org
Hannes, Ulan, Petr, please take a look. I've tested this on desktop and on a Svelte device and it doesn't seem to impact the existing measurements.
petrcermak@chromium.org changed reviewers: + nednguyen@google.com
+nednguyen (proper owner of tools/perf) LGTM with the following description nits: * s/v8_browse/v8_browsing/g (both title and description) * change patch title to "[tools/perf] Add CodeAndMetadata metrics to v8_browsing benchmarks and add Ignition variants". Thanks, Petr https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... File tools/perf/benchmarks/v8_browsing.py (right): https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... tools/perf/benchmarks/v8_browsing.py:69: platform=self.PLATFORM, case='browse') You don't need this line break ;-) https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... tools/perf/benchmarks/v8_browsing.py:90: class V8DesktopBrowsingBenchmark(_V8BrowsingBenchmark): You could remove some of the code duplication across the benchmarks by defining two abstract superclasses. I can see two options to do this: _V8MobileBrowsingBenchmark _V8DesktopBrowsingBenchmark -or- _V8NonIgnitionBrowsingBenchmark _V8IgnitionBrowsingBenchmark Perhaps the former would make more sense because there's more platform-related code (4 → 2 ShouldDisable methods). https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... tools/perf/benchmarks/v8_browsing.py:102: I ask you to add extra blank lines between top-level definitions below. Could you please add it here as well (even though it's not technically part of your patch)? https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... tools/perf/benchmarks/v8_browsing.py:111: nit: add blank line (there should be 2 blank lines between top-level definition) https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... tools/perf/benchmarks/v8_browsing.py:129: nit: add blank line (ditto)
Description was changed from ========== Add CodeAndMetadata metrics to v8_browse and add Ignition variants. Adds CodeAndMetadata measurements to the v8_browse telemetry benchmarks. Modifes the memory dumper to dump these stats on light dumps, if requested. Also adds an Ignition variant for v8_browse benchmarks. BUG=v8:4280,v8:5019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== Add CodeAndMetadata metrics to v8_browse and add Ignition variants. Adds CodeAndMetadata measurements to the v8_browse telemetry benchmarks. Modifes the memory dumper to dump these stats on light dumps, if requested. Also adds an Ignition variant for v8_browse benchmarks. BUG=v8:4280,v8:5019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
nednguyen@google.com changed reviewers: + nduca@chromium.org, sullivan@chromium.org
This add 2 extra benchmarks with the ignition flags, which is a lot. Should these benchmarks be removed when ignition is launched?
On 2016/08/19 11:14:54, nednguyen wrote: > This add 2 extra benchmarks with the ignition flags, which is a lot. Should > these benchmarks be removed when ignition is launched? It adds these, but I am also planning on removing two benchmarks when it lands (top_10_mobile_memory_ignition and top_10_mobile_memory). Also, it is only really one extra benchmark, since only one will run on a bot depending on whether it's mobile or desktop, so it should be a net win :). Once Ignition launches, I'll remove all the extra "_ignition" benchmarks.
LGTM if performance is not impacted.
On 2016/08/19 11:35:36, rmcilroy wrote: > On 2016/08/19 11:14:54, nednguyen wrote: > > This add 2 extra benchmarks with the ignition flags, which is a lot. Should > > these benchmarks be removed when ignition is launched? > > It adds these, but I am also planning on removing two benchmarks when it lands > (top_10_mobile_memory_ignition and top_10_mobile_memory). Also, it is only > really one extra benchmark, since only one will run on a bot depending on > whether it's mobile or desktop, so it should be a net win :). > > Once Ignition launches, I'll remove all the extra "_ignition" benchmarks. lgtm Can you move all the ignition benchmarks to benchmarks/v8_ignition.py to make it clearer which one to be removed once projects launched? :-)
Description was changed from ========== Add CodeAndMetadata metrics to v8_browse and add Ignition variants. Adds CodeAndMetadata measurements to the v8_browse telemetry benchmarks. Modifes the memory dumper to dump these stats on light dumps, if requested. Also adds an Ignition variant for v8_browse benchmarks. BUG=v8:4280,v8:5019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== [tools/perf] Add CodeAndMetadata metrics to v8_browsing benchmarks and add Ignition variants Adds CodeAndMetadata measurements to the v8_browsing telemetry benchmarks. Modifes the memory dumper to dump these stats on light dumps, if requested. Also adds an Ignition variant for v8_browsing benchmarks. BUG=v8:4280,v8:5019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
> Can you move all the ignition benchmarks to benchmarks/v8_ignition.py to make it > clearer which one to be removed once projects launched? :-) If you care about this strongly I can do it, but I'd prefer not to move them all, since they often reference "private" functions or classes of the base tests them self. When removing them I will remove v8_helper.EnableIgnition (which they all use) so it should be obvious which ones need to be removed. https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... File tools/perf/benchmarks/v8_browsing.py (right): https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... tools/perf/benchmarks/v8_browsing.py:69: platform=self.PLATFORM, case='browse') On 2016/08/19 11:06:29, petrcermak wrote: > You don't need this line break ;-) Done. https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... tools/perf/benchmarks/v8_browsing.py:90: class V8DesktopBrowsingBenchmark(_V8BrowsingBenchmark): On 2016/08/19 11:06:29, petrcermak wrote: > You could remove some of the code duplication across the benchmarks by defining > two abstract superclasses. I can see two options to do this: > > _V8MobileBrowsingBenchmark > _V8DesktopBrowsingBenchmark > > -or- > > _V8NonIgnitionBrowsingBenchmark > _V8IgnitionBrowsingBenchmark > > Perhaps the former would make more sense because there's more platform-related > code (4 → 2 ShouldDisable methods). Done. https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... tools/perf/benchmarks/v8_browsing.py:102: On 2016/08/19 11:06:30, petrcermak wrote: > I ask you to add extra blank lines between top-level definitions below. Could > you please add it here as well (even though it's not technically part of your > patch)? Done. https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... tools/perf/benchmarks/v8_browsing.py:111: On 2016/08/19 11:06:29, petrcermak wrote: > nit: add blank line (there should be 2 blank lines between top-level definition) Done. https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... tools/perf/benchmarks/v8_browsing.py:129: On 2016/08/19 11:06:29, petrcermak wrote: > nit: add blank line (ditto) Done.
rmcilroy@chromium.org changed reviewers: + jochen@chromium.org
+jochen@ for gin/ changes.
On 2016/08/19 15:38:59, rmcilroy wrote: > > Can you move all the ignition benchmarks to benchmarks/v8_ignition.py to make > it > > clearer which one to be removed once projects launched? :-) > > If you care about this strongly I can do it, but I'd prefer not to move them > all, since they often reference "private" functions or classes of the base tests > them self. When removing them I will remove v8_helper.EnableIgnition (which they > all use) so it should be obvious which ones need to be removed. Ah good point. I think we can make them public then, but I think the move should probably be done in a different CL. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=639316 about this. > > https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... > File tools/perf/benchmarks/v8_browsing.py (right): > > https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... > tools/perf/benchmarks/v8_browsing.py:69: platform=self.PLATFORM, case='browse') > On 2016/08/19 11:06:29, petrcermak wrote: > > You don't need this line break ;-) > > Done. > > https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... > tools/perf/benchmarks/v8_browsing.py:90: class > V8DesktopBrowsingBenchmark(_V8BrowsingBenchmark): > On 2016/08/19 11:06:29, petrcermak wrote: > > You could remove some of the code duplication across the benchmarks by > defining > > two abstract superclasses. I can see two options to do this: > > > > _V8MobileBrowsingBenchmark > > _V8DesktopBrowsingBenchmark > > > > -or- > > > > _V8NonIgnitionBrowsingBenchmark > > _V8IgnitionBrowsingBenchmark > > > > Perhaps the former would make more sense because there's more platform-related > > code (4 → 2 ShouldDisable methods). > > Done. > > https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... > tools/perf/benchmarks/v8_browsing.py:102: > On 2016/08/19 11:06:30, petrcermak wrote: > > I ask you to add extra blank lines between top-level definitions below. Could > > you please add it here as well (even though it's not technically part of your > > patch)? > > Done. > > https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... > tools/perf/benchmarks/v8_browsing.py:111: > On 2016/08/19 11:06:29, petrcermak wrote: > > nit: add blank line (there should be 2 blank lines between top-level > definition) > > Done. > > https://codereview.chromium.org/2257173003/diff/1/tools/perf/benchmarks/v8_br... > tools/perf/benchmarks/v8_browsing.py:129: > On 2016/08/19 11:06:29, petrcermak wrote: > > nit: add blank line (ditto) > > Done.
lgtm
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, petrcermak@chromium.org, hpayer@chromium.org Link to the patchset: https://codereview.chromium.org/2257173003/#ps20001 (title: "Address comments")
lgtm
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: mac_retina_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by rmcilroy@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: winx64_10_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64_10_perf_c...)
Description was changed from ========== [tools/perf] Add CodeAndMetadata metrics to v8_browsing benchmarks and add Ignition variants Adds CodeAndMetadata measurements to the v8_browsing telemetry benchmarks. Modifes the memory dumper to dump these stats on light dumps, if requested. Also adds an Ignition variant for v8_browsing benchmarks. BUG=v8:4280,v8:5019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== [tools/perf] Add CodeAndMetadata metrics to v8_browsing benchmarks and add Ignition variants Adds CodeAndMetadata measurements to the v8_browsing telemetry benchmarks. Modifes the memory dumper to dump these stats on light dumps, if requested. Also adds an Ignition variant for v8_browsing benchmarks. BUG=v8:4280,v8:5019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq; ==========
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [tools/perf] Add CodeAndMetadata metrics to v8_browsing benchmarks and add Ignition variants Adds CodeAndMetadata measurements to the v8_browsing telemetry benchmarks. Modifes the memory dumper to dump these stats on light dumps, if requested. Also adds an Ignition variant for v8_browsing benchmarks. BUG=v8:4280,v8:5019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq; ========== to ========== [tools/perf] Add CodeAndMetadata metrics to v8_browsing benchmarks and add Ignition variants Adds CodeAndMetadata measurements to the v8_browsing telemetry benchmarks. Modifes the memory dumper to dump these stats on light dumps, if requested. Also adds an Ignition variant for v8_browsing benchmarks. BUG=v8:4280,v8:5019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq; ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [tools/perf] Add CodeAndMetadata metrics to v8_browsing benchmarks and add Ignition variants Adds CodeAndMetadata measurements to the v8_browsing telemetry benchmarks. Modifes the memory dumper to dump these stats on light dumps, if requested. Also adds an Ignition variant for v8_browsing benchmarks. BUG=v8:4280,v8:5019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq; ========== to ========== [tools/perf] Add CodeAndMetadata metrics to v8_browsing benchmarks and add Ignition variants Adds CodeAndMetadata measurements to the v8_browsing telemetry benchmarks. Modifes the memory dumper to dump these stats on light dumps, if requested. Also adds an Ignition variant for v8_browsing benchmarks. BUG=v8:4280,v8:5019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq; Committed: https://crrev.com/6b6b89ace1fded16dd2901e2de16b0346aeb3e5a Cr-Commit-Position: refs/heads/master@{#414439} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6b6b89ace1fded16dd2901e2de16b0346aeb3e5a Cr-Commit-Position: refs/heads/master@{#414439} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
