Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1907343002:
Set up a parallel memory.memory_health_quick_tbmv2 benchmark (Closed)
|
Created:
4 years, 8 months ago by petrcermak Modified:
4 years, 8 months ago 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. |
DescriptionSet up a parallel memory.memory_health_quick_tbmv2 benchmark
This patch adds a temporary parallel benchmark to compare the new TBMv2
memory metric (third_party/catapult/tracing/tracing/metrics/
system_health/memory_metric.html) with the existing TBMv1 one
(third_party/catapult/telemetry/telemetry/web_perf/metrics/
memory_timeline.py).
Once all issues associated with the TBMv2 metric are resolved, all
memory benchmarks will switch to use it instead of the TBMv1 metric and
the temporary parallel benchmark will be removed.
BUG=581716, 606361
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq
Committed: https://crrev.com/92efb5bc7f3830436cc047a39e79c5374b5a4d7a
Cr-Commit-Position: refs/heads/master@{#389519}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address Juan's comments #
Total comments: 2
Patch Set 3 : Rename to memory.top_10_mobile_tbmv2 #
Total comments: 2
Patch Set 4 : Add bug context #
Messages
Total messages: 22 (8 generated)
Description was changed from ========== Set up a parallel memory.memory_health_quick_tbmv2 benchmark This patch adds a temporary parallel benchmark to compare the new TBMv2 memory metric (third_party/catapult/tracing/tracing/metrics/ system_health/memory_metric.html) with the existing TBMv1 one (third_party/catapult/telemetry/telemetry/web_perf/metrics/ memory_timeline.py). Once all issues associated with the TBMv2 metric are resolved, all memory benchmarks will switch to use it instead of the TBMv1 metric and the temporary parallel benchmark will be removed. BUG=581716 ========== to ========== Set up a parallel memory.memory_health_quick_tbmv2 benchmark This patch adds a temporary parallel benchmark to compare the new TBMv2 memory metric (third_party/catapult/tracing/tracing/metrics/ system_health/memory_metric.html) with the existing TBMv1 one (third_party/catapult/telemetry/telemetry/web_perf/metrics/ memory_timeline.py). Once all issues associated with the TBMv2 metric are resolved, all memory benchmarks will switch to use it instead of the TBMv1 metric and the temporary parallel benchmark will be removed. BUG=581716 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq ==========
petrcermak@chromium.org changed reviewers: + nednguyen@google.com, perezju@chromium.org, primiano@chromium.org
PTAL. Thanks, Petr
lgtm
https://codereview.chromium.org/1907343002/diff/1/tools/perf/benchmarks/memor... File tools/perf/benchmarks/memory_infra.py (right): https://codereview.chromium.org/1907343002/diff/1/tools/perf/benchmarks/memor... tools/perf/benchmarks/memory_infra.py:33: class _TBMv1MemoryInfra(_MemoryInfra): As per the discussion in https://github.com/catapult-project/catapult/issues/2137#issuecomment-197954711 we're planning to get rid of these "TraceRerun" methods; so let's take the chance to do a bit of clean up here. I would suggest. - Get rid of HasTraceRerunDebugOption. - Get rid of SetupBenchmarkDefaultTraceRerunOptions, its code can be moved to the bottom of CreateTimelineBasedMeasurementOptions. After doing this, the change between v1 and v2 is so small (just calling SetLegacyTimelineBasedMetrics vs SeTimelineBasedMetrics), that probably isn't worth anymore to have two different classes. Here is a suggestion: - Just add a class variable, something like 'USE_LEGACY_TBM' that defaults to True; the class uses this to decide which tbm setting method to call. - Then TBMv2MemoryHealthQuick can just subclass MemoryHealthQuick, flip the value of that variable, and adjust the name. This also avoids the awkward bit below where one of the classes appears to be "hijacking" methods from the other; now it just would be "super". https://codereview.chromium.org/1907343002/diff/1/tools/perf/page_sets/memory... File tools/perf/page_sets/memory_health_story.py (right): https://codereview.chromium.org/1907343002/diff/1/tools/perf/page_sets/memory... tools/perf/page_sets/memory_health_story.py:45: with action_runner.CreateInteraction(self._PHASE): what happens to the interaction record in TBMv2? It's just ignored?
Thanks for your comments. PTAL. Petr https://codereview.chromium.org/1907343002/diff/1/tools/perf/benchmarks/memor... File tools/perf/benchmarks/memory_infra.py (right): https://codereview.chromium.org/1907343002/diff/1/tools/perf/benchmarks/memor... tools/perf/benchmarks/memory_infra.py:33: class _TBMv1MemoryInfra(_MemoryInfra): On 2016/04/25 09:03:49, perezju wrote: > As per the discussion in > https://github.com/catapult-project/catapult/issues/2137#issuecomment-197954711 > we're planning to get rid of these "TraceRerun" methods; so let's take the > chance to do a bit of clean up here. > > I would suggest. > - Get rid of HasTraceRerunDebugOption. > - Get rid of SetupBenchmarkDefaultTraceRerunOptions, its code can be moved to > the bottom of CreateTimelineBasedMeasurementOptions. > > After doing this, the change between v1 and v2 is so small (just calling > SetLegacyTimelineBasedMetrics vs SeTimelineBasedMetrics), that probably isn't > worth anymore to have two different classes. > > Here is a suggestion: > - Just add a class variable, something like 'USE_LEGACY_TBM' that defaults to > True; the class uses this to decide which tbm setting method to call. > - Then TBMv2MemoryHealthQuick can just subclass MemoryHealthQuick, flip the > value of that variable, and adjust the name. This also avoids the awkward bit > below where one of the classes appears to be "hijacking" methods from the other; > now it just would be "super". > Done. https://codereview.chromium.org/1907343002/diff/1/tools/perf/page_sets/memory... File tools/perf/page_sets/memory_health_story.py (right): https://codereview.chromium.org/1907343002/diff/1/tools/perf/page_sets/memory... tools/perf/page_sets/memory_health_story.py:45: with action_runner.CreateInteraction(self._PHASE): On 2016/04/25 09:03:49, perezju wrote: > what happens to the interaction record in TBMv2? It's just ignored? Yes, it's just ignored. FYI, I had to land https://codereview.chromium.org/1912173003/ so that the story would still work with TBMv1.
lgtm but also would like to check with Primiano: is it enough to just test with the "quick" version of the benchmark; or would we also want to run full version on internal bots? Note: this could be run in parallel on a separate device, so would not increase running time. https://codereview.chromium.org/1907343002/diff/1/tools/perf/benchmarks/memor... File tools/perf/benchmarks/memory_infra.py (right): https://codereview.chromium.org/1907343002/diff/1/tools/perf/benchmarks/memor... tools/perf/benchmarks/memory_infra.py:33: class _TBMv1MemoryInfra(_MemoryInfra): On 2016/04/25 10:00:35, petrcermak wrote: > On 2016/04/25 09:03:49, perezju wrote: > > As per the discussion in > > > https://github.com/catapult-project/catapult/issues/2137#issuecomment-197954711 > > we're planning to get rid of these "TraceRerun" methods; so let's take the > > chance to do a bit of clean up here. > > > > I would suggest. > > - Get rid of HasTraceRerunDebugOption. > > - Get rid of SetupBenchmarkDefaultTraceRerunOptions, its code can be moved to > > the bottom of CreateTimelineBasedMeasurementOptions. > > > > After doing this, the change between v1 and v2 is so small (just calling > > SetLegacyTimelineBasedMetrics vs SeTimelineBasedMetrics), that probably isn't > > worth anymore to have two different classes. > > > > Here is a suggestion: > > - Just add a class variable, something like 'USE_LEGACY_TBM' that defaults to > > True; the class uses this to decide which tbm setting method to call. > > - Then TBMv2MemoryHealthQuick can just subclass MemoryHealthQuick, flip the > > value of that variable, and adjust the name. This also avoids the awkward bit > > below where one of the classes appears to be "hijacking" methods from the > other; > > now it just would be "super". > > > > Done. Awesome! Looks much better.
See discussion on crbug.com/606361 about naming. If everybody agrees there, this should be called memory.top_10_mobile_tbmv2 and use the top_10_mobile pageset.
(resending as I forgot a draft) See discussion on crbug.com/606361 about naming. If everybody agrees there, this should be called memory.top_10_mobile_tbmv2 and use the top_10_mobile pageset. https://codereview.chromium.org/1907343002/diff/20001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory_infra.py (right): https://codereview.chromium.org/1907343002/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory_infra.py:25: # Subclasses can override this to use TBMv2 instead of TBMv1. Should this say: TMBv1 instead of TBMv2 (IIUC True == v1, False== v2)
PTAL. Thanks, Petr https://codereview.chromium.org/1907343002/diff/20001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory_infra.py (right): https://codereview.chromium.org/1907343002/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory_infra.py:25: # Subclasses can override this to use TBMv2 instead of TBMv1. On 2016/04/25 15:31:03, Primiano Tucci wrote: > Should this say: TMBv1 instead of TBMv2 (IIUC True == v1, False== v2) I changed this to be an integer argument to make it easier to understand.
LGTM, just add a link to crbug.com/606361 https://codereview.chromium.org/1907343002/diff/40001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory_infra.py (right): https://codereview.chromium.org/1907343002/diff/40001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory_infra.py:95: of the TBMv1 metric and this temporary benchmark will be removed. Add a link to crbug.com/606361 which has all the context plz.
Description was changed from ========== Set up a parallel memory.memory_health_quick_tbmv2 benchmark This patch adds a temporary parallel benchmark to compare the new TBMv2 memory metric (third_party/catapult/tracing/tracing/metrics/ system_health/memory_metric.html) with the existing TBMv1 one (third_party/catapult/telemetry/telemetry/web_perf/metrics/ memory_timeline.py). Once all issues associated with the TBMv2 metric are resolved, all memory benchmarks will switch to use it instead of the TBMv1 metric and the temporary parallel benchmark will be removed. BUG=581716 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq ========== to ========== Set up a parallel memory.memory_health_quick_tbmv2 benchmark This patch adds a temporary parallel benchmark to compare the new TBMv2 memory metric (third_party/catapult/tracing/tracing/metrics/ system_health/memory_metric.html) with the existing TBMv1 one (third_party/catapult/telemetry/telemetry/web_perf/metrics/ memory_timeline.py). Once all issues associated with the TBMv2 metric are resolved, all memory benchmarks will switch to use it instead of the TBMv1 metric and the temporary parallel benchmark will be removed. BUG=581716,606361 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq ==========
Patchset #4 (id:60001) has been deleted
Thanks for your comments. Petr https://codereview.chromium.org/1907343002/diff/40001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory_infra.py (right): https://codereview.chromium.org/1907343002/diff/40001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory_infra.py:95: of the TBMv1 metric and this temporary benchmark will be removed. On 2016/04/25 16:14:55, Primiano Tucci wrote: > Add a link to crbug.com/606361 which has all the context plz. Done.
The CQ bit was checked by petrcermak@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, perezju@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1907343002/#ps80001 (title: "Add bug context")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907343002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907343002/80001
Message was sent while issue was closed.
Description was changed from ========== Set up a parallel memory.memory_health_quick_tbmv2 benchmark This patch adds a temporary parallel benchmark to compare the new TBMv2 memory metric (third_party/catapult/tracing/tracing/metrics/ system_health/memory_metric.html) with the existing TBMv1 one (third_party/catapult/telemetry/telemetry/web_perf/metrics/ memory_timeline.py). Once all issues associated with the TBMv2 metric are resolved, all memory benchmarks will switch to use it instead of the TBMv1 metric and the temporary parallel benchmark will be removed. BUG=581716,606361 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq ========== to ========== Set up a parallel memory.memory_health_quick_tbmv2 benchmark This patch adds a temporary parallel benchmark to compare the new TBMv2 memory metric (third_party/catapult/tracing/tracing/metrics/ system_health/memory_metric.html) with the existing TBMv1 one (third_party/catapult/telemetry/telemetry/web_perf/metrics/ memory_timeline.py). Once all issues associated with the TBMv2 metric are resolved, all memory benchmarks will switch to use it instead of the TBMv1 metric and the temporary parallel benchmark will be removed. BUG=581716,606361 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Set up a parallel memory.memory_health_quick_tbmv2 benchmark This patch adds a temporary parallel benchmark to compare the new TBMv2 memory metric (third_party/catapult/tracing/tracing/metrics/ system_health/memory_metric.html) with the existing TBMv1 one (third_party/catapult/telemetry/telemetry/web_perf/metrics/ memory_timeline.py). Once all issues associated with the TBMv2 metric are resolved, all memory benchmarks will switch to use it instead of the TBMv1 metric and the temporary parallel benchmark will be removed. BUG=581716,606361 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq ========== to ========== Set up a parallel memory.memory_health_quick_tbmv2 benchmark This patch adds a temporary parallel benchmark to compare the new TBMv2 memory metric (third_party/catapult/tracing/tracing/metrics/ system_health/memory_metric.html) with the existing TBMv1 one (third_party/catapult/telemetry/telemetry/web_perf/metrics/ memory_timeline.py). Once all issues associated with the TBMv2 metric are resolved, all memory benchmarks will switch to use it instead of the TBMv1 metric and the temporary parallel benchmark will be removed. BUG=581716,606361 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq Committed: https://crrev.com/92efb5bc7f3830436cc047a39e79c5374b5a4d7a Cr-Commit-Position: refs/heads/master@{#389519} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/92efb5bc7f3830436cc047a39e79c5374b5a4d7a Cr-Commit-Position: refs/heads/master@{#389519}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:80001) has been created in https://codereview.chromium.org/1915163003/ by perezju@chromium.org. The reason for reverting is: Broke data upload to perf dashboard for these benchmarks. BUG=606698. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
