Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 months, 1 week ago by petrcermak
Modified:
11 months ago
Reviewers:
nednguyen, charliea, perezju
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 #

Messages

Total messages: 25 (11 generated)
petrcermak
PTAL. Note that system health benchmarks will now effectively be waiting for 16 seconds after ...
11 months, 1 week 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 ...
11 months, 1 week 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: > ...
11 months, 1 week 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: > ...
11 months, 1 week 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: > ...
11 months, 1 week ago (2016-08-15 15:04:02 UTC) #6
petrcermak
PTAL. Thanks, Petr
11 months, 1 week ago (2016-08-16 18:09:37 UTC) #11
nednguyen
lgtm
11 months, 1 week 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: > ...
11 months, 1 week ago (2016-08-16 19:55:30 UTC) #13
charliea
lgtm
11 months, 1 week 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 ...
11 months, 1 week 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
11 months ago (2016-08-19 10:47:42 UTC) #20
commit-bot: I haz the power
Committed patchset #3 (id:40001)
11 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}
11 months ago (2016-08-19 13:59:01 UTC) #24
petrcermak
11 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973