Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(172)

Issue 2248693002: [system-health] Wait for 10 seconds after each story (except for memory benchmarks) (Closed)

Created:
4 years, 4 months ago by petrcermak
Modified:
4 years, 4 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.

Description

[system-health] Wait for 10 seconds after each story (except for memory benchmarks) Non-memory SH benchmarks: All SH stories will wait for 10 seconds after loading. Memory SH benchmarks: All SH stories will re-use action_runner.MeasureMemory(). BUG=589726, 637158 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 Committed: https://crrev.com/4cd6d6ceb622b79120215cae46df22f8abe68466 Cr-Commit-Position: refs/heads/master@{#413128}

Patch Set 1 #

Patch Set 2 : Don't modify memory benchmarks yet #

Total comments: 2

Patch Set 3 : Add comment & TODO #

Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -13 lines) Patch
M tools/perf/page_sets/system_health/system_health_story.py View 1 2 2 chunks +19 lines, -13 lines 0 comments Download

Messages

Total messages: 25 (11 generated)
petrcermak
PTAL. Note that system health benchmarks will now effectively be waiting for 16 seconds after ...
4 years, 4 months ago (2016-08-15 13:37:42 UTC) #2
nednguyen
On 2016/08/15 13:37:42, petrcermak wrote: > PTAL. > > Note that system health benchmarks will ...
4 years, 4 months ago (2016-08-15 13:48:32 UTC) #3
petrcermak
On 2016/08/15 13:48:32, nednguyen (ooo til 8-13) wrote: > On 2016/08/15 13:37:42, petrcermak wrote: > ...
4 years, 4 months ago (2016-08-15 13:54:20 UTC) #4
nednguyen
On 2016/08/15 13:54:20, petrcermak wrote: > On 2016/08/15 13:48:32, nednguyen (ooo til 8-13) wrote: > ...
4 years, 4 months ago (2016-08-15 13:59:16 UTC) #5
petrcermak
On 2016/08/15 13:59:16, nednguyen (ooo til 8-13) wrote: > On 2016/08/15 13:54:20, petrcermak wrote: > ...
4 years, 4 months ago (2016-08-15 15:04:02 UTC) #6
petrcermak
PTAL. Thanks, Petr
4 years, 4 months ago (2016-08-16 18:09:37 UTC) #11
nednguyen
lgtm
4 years, 4 months ago (2016-08-16 18:17:35 UTC) #12
chromium-reviews
On Tue, 16 Aug 2016 at 11:24, Juan Antonio Navarro Pérez < perezju@chromium.org> wrote: > ...
4 years, 4 months ago (2016-08-16 19:55:30 UTC) #13
charliea (OOO until 10-5)
lgtm
4 years, 4 months ago (2016-08-16 20:26:42 UTC) #16
perezju
lgtm but I think that ShouldMeasureMemory is not needed, since the check of whether tracing ...
4 years, 4 months ago (2016-08-17 16:24:45 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2248693002/40001
4 years, 4 months ago (2016-08-19 10:47:42 UTC) #20
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 4 months ago (2016-08-19 13:56:16 UTC) #22
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/4cd6d6ceb622b79120215cae46df22f8abe68466 Cr-Commit-Position: refs/heads/master@{#413128}
4 years, 4 months ago (2016-08-19 13:59:01 UTC) #24
petrcermak
4 years, 4 months ago (2016-08-23 14:34:40 UTC) #25
Message was sent while issue was closed.
I've just realized that I haven't sent the reply to Juan's comment.

https://codereview.chromium.org/2248693002/diff/20001/tools/perf/page_sets/sy...
File tools/perf/page_sets/system_health/system_health_story.py (right):

https://codereview.chromium.org/2248693002/diff/20001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/system_health_story.py:61: return True
On 2016/08/17 16:24:45, perezju wrote:
> This check is already happening within MeasureMemory.

We actually need this for when we record the stories (they currently can't be
recorded through the storyset itself because it has extra constructor
parameters). I'll add a TODO to fix this.

Powered by Google App Engine
This is Rietveld 408576698