Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1773103002:
Replace memory.long_running_idle_gmail with TBM based version. (Closed)
DescriptionReplace memory.long_running_idle_gmail with TBM based version.
The new benchmark uses memory-infra to get memory stats and also
tracks GC pause times.
BUG=
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/6c8f67a2512734a08a8f28878b2caf6b65151cdb
Cr-Commit-Position: refs/heads/master@{#380400}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove ValueCanBeAddedPredicate #
Total comments: 8
Patch Set 3 : Move to memory-infra #
Total comments: 4
Patch Set 4 : Address comments from perezju #
Messages
Total messages: 22 (7 generated)
Description was changed from ========== Replace memory.long_running_idle_gmail with TBM based version. The new benchmark uses memory-infra to get memory stats and also tracks GC pause times. BUG= ========== to ========== Replace memory.long_running_idle_gmail with TBM based version. The new benchmark uses memory-infra to get memory stats and also tracks GC pause times. BUG= 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 ==========
ulan@chromium.org changed reviewers: + sullivan@chromium.org
PTAL
sullivan@chromium.org changed reviewers: + eakuefner@chromium.org, nednguyen@google.com
I'm really excited to see this! But reassigning the review to Ned and Ethan who have been working on TBM more.
nednguyen@google.com changed reviewers: + perezju@chromium.org, petrcermak@chromium.org
I defer reviewing this to memory-infra team & Ethan.
This is exciting! Can you talk about the noise characteristics of this reimplementation? With memory-infra the memory metrics should be much better, and I assume the V8 timeline-based metric is better as well, but I'd just like to confirm that this doesn't regress noise. Also, do you find that this benchmark runs without any tracing problems? Memory infra folks, do you think that there will be any issues with idling for 100 seconds while tracing? https://codereview.chromium.org/1773103002/diff/1/tools/perf/benchmarks/memor... File tools/perf/benchmarks/memory.py (right): https://codereview.chromium.org/1773103002/diff/1/tools/perf/benchmarks/memor... tools/perf/benchmarks/memory.py:79: def ValueCanBeAddedPredicate(cls, value, is_first_result): Why do you do this filtration? Can you just remove this? If this is about trying to reduce the number of values you need to scroll through on the perf dashboard, know that the new report page has fuzzy autocomplete per-field, so uploading extraneous numbers should not be a big deal.
Thanks for the quick response. > Can you talk about the noise characteristics of this reimplementation? With > memory-infra the memory metrics should be much better, and I assume the V8 > timeline-based metric is better as well, but I'd just like to confirm that this > doesn't regress noise. The old benchmark was tracking only memory metrics. The new version tracks memory and GC time/counters. This is cool because it made obvious that the benchmark is bi-modal: sometimes it does 5 GCs and sometimes 3 GCs. The memory noise comes from these modes and is at the same level as before. Within each mode though, the new memory metrics are very stable (~2% difference between runs). > Also, do you find that this benchmark runs without any tracing problems? Memory > infra folks, do you think that there will be any issues with idling for 100 > seconds while tracing? It runs OK on Linux. Not sure about other platforms. > https://codereview.chromium.org/1773103002/diff/1/tools/perf/benchmarks/memor... > File tools/perf/benchmarks/memory.py (right): > > https://codereview.chromium.org/1773103002/diff/1/tools/perf/benchmarks/memor... > tools/perf/benchmarks/memory.py:79: def ValueCanBeAddedPredicate(cls, value, > is_first_result): > Why do you do this filtration? Can you just remove this? > > If this is about trying to reduce the number of values you need to scroll > through on the perf dashboard, know that the new report page has fuzzy > autocomplete per-field, so uploading extraneous numbers should not be a big > deal. This is to keep the local results.html file compact, i.e. fitting in one screen, which is great for quickly comparing local Chrome changes.
On 2016/03/08 at 19:22:16, ulan wrote: > Thanks for the quick response. > > > Can you talk about the noise characteristics of this reimplementation? With > > memory-infra the memory metrics should be much better, and I assume the V8 > > timeline-based metric is better as well, but I'd just like to confirm that this > > doesn't regress noise. > The old benchmark was tracking only memory metrics. The new version tracks memory and GC time/counters. > This is cool because it made obvious that the benchmark is bi-modal: sometimes it does 5 GCs and sometimes 3 GCs. > The memory noise comes from these modes and is at the same level as before. > Within each mode though, the new memory metrics are very stable (~2% difference between runs). > > > Also, do you find that this benchmark runs without any tracing problems? Memory > > infra folks, do you think that there will be any issues with idling for 100 > > seconds while tracing? > It runs OK on Linux. Not sure about other platforms. > > > https://codereview.chromium.org/1773103002/diff/1/tools/perf/benchmarks/memor... > > File tools/perf/benchmarks/memory.py (right): > > > > https://codereview.chromium.org/1773103002/diff/1/tools/perf/benchmarks/memor... > > tools/perf/benchmarks/memory.py:79: def ValueCanBeAddedPredicate(cls, value, > > is_first_result): > > Why do you do this filtration? Can you just remove this? > > > > If this is about trying to reduce the number of values you need to scroll > > through on the perf dashboard, know that the new report page has fuzzy > > autocomplete per-field, so uploading extraneous numbers should not be a big > > deal. > > This is to keep the local results.html file compact, i.e. fitting in one screen, which is great for quickly comparing local Chrome changes. Sorry about results.html -- we have plans to revamp the template to make it more actionable and configurable, but unfortunately they depend on a lot of moving pieces that are currently in flux. Please let's not do this if it's only for the sake of this UI tweak. The values you omit may be useful for debugging and regression detection later on, and using ValueCanBeAddedPredicate will make porting to TBMv2 less one-to-one.
> Sorry about results.html -- we have plans to revamp the template to make it more > actionable and configurable, but unfortunately they depend on a lot of moving > pieces that are currently in flux. > > Please let's not do this if it's only for the sake of this UI tweak. The values > you omit may be useful for debugging and regression detection later on, and > using ValueCanBeAddedPredicate will make porting to TBMv2 less one-to-one. Thanks for explaining. I removed the predicate.
https://codereview.chromium.org/1773103002/diff/20001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory.py (right): https://codereview.chromium.org/1773103002/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory.py:47: @benchmark.Disabled('reference', 'android') # crbug.com/579546 just double checking, why disabling this test on android? https://codereview.chromium.org/1773103002/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory.py:48: class MemoryLongRunningIdleGmailTBM(perf_benchmark.PerfBenchmark): maybe move benchmark to memory_infra.py and subclass from _MemoryInfra benchmark? (that also helps to avoid duplicating the workaround in SetExtraBrowserOptions below) https://codereview.chromium.org/1773103002/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory.py:69: options.SetLegacyTimelineBasedMetrics([ maybe this should be moved to SetupBenchmarkDefaultTraceRerunOptions, so that running in debug mode still records values for all metrics.
Thanks for the comments. I uploaded new patch set. https://codereview.chromium.org/1773103002/diff/20001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory.py (right): https://codereview.chromium.org/1773103002/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory.py:47: @benchmark.Disabled('reference', 'android') # crbug.com/579546 On 2016/03/09 15:07:55, perezju wrote: > just double checking, why disabling this test on android? The page is too heavy for android, fails with OOM. https://codereview.chromium.org/1773103002/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory.py:48: class MemoryLongRunningIdleGmailTBM(perf_benchmark.PerfBenchmark): On 2016/03/09 15:07:55, perezju wrote: > maybe move benchmark to memory_infra.py and subclass from _MemoryInfra > benchmark? (that also helps to avoid duplicating the workaround in > SetExtraBrowserOptions below) Done. https://codereview.chromium.org/1773103002/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory.py:69: options.SetLegacyTimelineBasedMetrics([ On 2016/03/09 15:07:55, perezju wrote: > maybe this should be moved to SetupBenchmarkDefaultTraceRerunOptions, so that > running in debug mode still records values for all metrics. I tried that, but the method was not being called at all.
lgtm when juan is happy
lgtm w/nits https://codereview.chromium.org/1773103002/diff/20001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory.py (right): https://codereview.chromium.org/1773103002/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory.py:69: options.SetLegacyTimelineBasedMetrics([ On 2016/03/09 17:18:39, ulan wrote: > On 2016/03/09 15:07:55, perezju wrote: > > maybe this should be moved to SetupBenchmarkDefaultTraceRerunOptions, so that > > running in debug mode still records values for all metrics. > > I tried that, but the method was not being called at all. Hmm, you're right. SetupTraceRerunOptions is only called for old style PageTest benchmarks, and not for new TBM ones (c.f. [1]). Please leave a TODO here and file a bug on me to get this fixed, as this is also broken for other benchmarks above. [1]: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... https://codereview.chromium.org/1773103002/diff/40001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory.py (left): https://codereview.chromium.org/1773103002/diff/40001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory.py:11: nit: I guess this blank line should stay there https://codereview.chromium.org/1773103002/diff/40001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory_infra.py (right): https://codereview.chromium.org/1773103002/diff/40001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory_infra.py:123: @benchmark.Disabled('reference', 'android') # crbug.com/579546 nit: add a short sentence explaining why it's disabled on android (like it's done with the sentence explaining why it's disabled on reference)
https://codereview.chromium.org/1773103002/diff/20001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory.py (right): https://codereview.chromium.org/1773103002/diff/20001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory.py:69: options.SetLegacyTimelineBasedMetrics([ On 2016/03/10 09:34:36, perezju wrote: > On 2016/03/09 17:18:39, ulan wrote: > > On 2016/03/09 15:07:55, perezju wrote: > > > maybe this should be moved to SetupBenchmarkDefaultTraceRerunOptions, so > that > > > running in debug mode still records values for all metrics. > > > > I tried that, but the method was not being called at all. > > Hmm, you're right. SetupTraceRerunOptions is only called for old style PageTest > benchmarks, and not for new TBM ones (c.f. [1]). Please leave a TODO here and > file a bug on me to get this fixed, as this is also broken for other benchmarks > above. > > [1]: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... Done. https://codereview.chromium.org/1773103002/diff/40001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory.py (left): https://codereview.chromium.org/1773103002/diff/40001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory.py:11: On 2016/03/10 09:34:36, perezju wrote: > nit: I guess this blank line should stay there Done. https://codereview.chromium.org/1773103002/diff/40001/tools/perf/benchmarks/m... File tools/perf/benchmarks/memory_infra.py (right): https://codereview.chromium.org/1773103002/diff/40001/tools/perf/benchmarks/m... tools/perf/benchmarks/memory_infra.py:123: @benchmark.Disabled('reference', 'android') # crbug.com/579546 On 2016/03/10 09:34:36, perezju wrote: > nit: add a short sentence explaining why it's disabled on android (like it's > done with the sentence explaining why it's disabled on reference) Done.
The CQ bit was checked by ulan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, perezju@chromium.org Link to the patchset: https://codereview.chromium.org/1773103002/#ps60001 (title: "Address comments from perezju")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773103002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773103002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Replace memory.long_running_idle_gmail with TBM based version. The new benchmark uses memory-infra to get memory stats and also tracks GC pause times. BUG= 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 ========== Replace memory.long_running_idle_gmail with TBM based version. The new benchmark uses memory-infra to get memory stats and also tracks GC pause times. BUG= 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/6c8f67a2512734a08a8f28878b2caf6b65151cdb Cr-Commit-Position: refs/heads/master@{#380400} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6c8f67a2512734a08a8f28878b2caf6b65151cdb Cr-Commit-Position: refs/heads/master@{#380400} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
