|
|
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 #Messages
Total messages: 25 (11 generated)
petrcermak@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com, perezju@chromium.org
PTAL. Note that system health benchmarks will now effectively be waiting for 16 seconds after loading: SystemHealthStory._Measure: 10 seconds ActionRunner.MeasureMemory: 3 seconds (BEFORE flushing system cache) ActionRunner.MeasureMemory: 3 seconds (AFTER flushing system cache) We could probably drop the first 3 seconds (BEFORE), which would bring this down to 13 seconds. WDYT? Note that this would require adding another argument to ActionRunner.MeasureMemory. Thanks, Petr
On 2016/08/15 13:37:42, petrcermak wrote: > PTAL. > > Note that system health benchmarks will now effectively be waiting for 16 > seconds after loading: > > SystemHealthStory._Measure: 10 seconds > ActionRunner.MeasureMemory: 3 seconds (BEFORE flushing system cache) > ActionRunner.MeasureMemory: 3 seconds (AFTER flushing system cache) > > We could probably drop the first 3 seconds (BEFORE), which would bring this down > to 13 seconds. WDYT? Note that this would require adding another argument to > ActionRunner.MeasureMemory. > > Thanks, > Petr Can we make the total wait time after load be roughly the same for memory vs non-memory? e.g: Non memory: StartNavigate() Wait(10) Memory: StartNavigate() start = time.time() TakeMemoryMeasurement() memory_time = time.time() - start Wait(10 - memory_time)
On 2016/08/15 13:48:32, nednguyen (ooo til 8-13) wrote: > On 2016/08/15 13:37:42, petrcermak wrote: > > PTAL. > > > > Note that system health benchmarks will now effectively be waiting for 16 > > seconds after loading: > > > > SystemHealthStory._Measure: 10 seconds > > ActionRunner.MeasureMemory: 3 seconds (BEFORE flushing system cache) > > ActionRunner.MeasureMemory: 3 seconds (AFTER flushing system cache) > > > > We could probably drop the first 3 seconds (BEFORE), which would bring this > down > > to 13 seconds. WDYT? Note that this would require adding another argument to > > ActionRunner.MeasureMemory. > > > > Thanks, > > Petr > > Can we make the total wait time after load be roughly the same for memory vs > non-memory? e.g: > > Non memory: > > StartNavigate() > Wait(10) > > Memory: > StartNavigate() > start = time.time() > TakeMemoryMeasurement() > memory_time = time.time() - start > Wait(10 - memory_time) This would make the remaining wait time completely useless. However, I do agree that memory should take roughly as much as non-memory. Ideally, we should do the opposite: wait for |10 - memory_time| seconds and then take memory measurement. To do that (approximately), we could do: if measure_memory: Wait(4) MeasureMemory() else: Wait(10) To avoid leaking the internals of MeasureMemory, we could do the following instead: if measure_memory: MeasureMemory(wait=10) else: Wait(10)
On 2016/08/15 13:54:20, petrcermak wrote: > On 2016/08/15 13:48:32, nednguyen (ooo til 8-13) wrote: > > On 2016/08/15 13:37:42, petrcermak wrote: > > > PTAL. > > > > > > Note that system health benchmarks will now effectively be waiting for 16 > > > seconds after loading: > > > > > > SystemHealthStory._Measure: 10 seconds > > > ActionRunner.MeasureMemory: 3 seconds (BEFORE flushing system cache) > > > ActionRunner.MeasureMemory: 3 seconds (AFTER flushing system cache) > > > > > > We could probably drop the first 3 seconds (BEFORE), which would bring this > > down > > > to 13 seconds. WDYT? Note that this would require adding another argument to > > > ActionRunner.MeasureMemory. > > > > > > Thanks, > > > Petr > > > > Can we make the total wait time after load be roughly the same for memory vs > > non-memory? e.g: > > > > Non memory: > > > > StartNavigate() > > Wait(10) > > > > Memory: > > StartNavigate() > > start = time.time() > > TakeMemoryMeasurement() > > memory_time = time.time() - start > > Wait(10 - memory_time) > > This would make the remaining wait time completely useless. However, I do agree > that memory should take roughly as much as non-memory. Ideally, we should do the > opposite: wait for |10 - memory_time| seconds and then take memory measurement. > To do that (approximately), we could do: > > if measure_memory: > Wait(4) > MeasureMemory() > else: > Wait(10) > > To avoid leaking the internals of MeasureMemory, we could do the following > instead: > > if measure_memory: > MeasureMemory(wait=10) > else: > Wait(10) sgtm I was suggesting keeping the timing of memory measurement as before because I worried it may trigger a bunch of false alert on memory. I leave it to you to make the final decision.
On 2016/08/15 13:59:16, nednguyen (ooo til 8-13) wrote: > On 2016/08/15 13:54:20, petrcermak wrote: > > On 2016/08/15 13:48:32, nednguyen (ooo til 8-13) wrote: > > > On 2016/08/15 13:37:42, petrcermak wrote: > > > > PTAL. > > > > > > > > Note that system health benchmarks will now effectively be waiting for 16 > > > > seconds after loading: > > > > > > > > SystemHealthStory._Measure: 10 seconds > > > > ActionRunner.MeasureMemory: 3 seconds (BEFORE flushing system cache) > > > > ActionRunner.MeasureMemory: 3 seconds (AFTER flushing system cache) > > > > > > > > We could probably drop the first 3 seconds (BEFORE), which would bring > this > > > down > > > > to 13 seconds. WDYT? Note that this would require adding another argument > to > > > > ActionRunner.MeasureMemory. > > > > > > > > Thanks, > > > > Petr > > > > > > Can we make the total wait time after load be roughly the same for memory vs > > > non-memory? e.g: > > > > > > Non memory: > > > > > > StartNavigate() > > > Wait(10) > > > > > > Memory: > > > StartNavigate() > > > start = time.time() > > > TakeMemoryMeasurement() > > > memory_time = time.time() - start > > > Wait(10 - memory_time) > > > > This would make the remaining wait time completely useless. However, I do > agree > > that memory should take roughly as much as non-memory. Ideally, we should do > the > > opposite: wait for |10 - memory_time| seconds and then take memory > measurement. > > To do that (approximately), we could do: > > > > if measure_memory: > > Wait(4) > > MeasureMemory() > > else: > > Wait(10) > > > > To avoid leaking the internals of MeasureMemory, we could do the following > > instead: > > > > if measure_memory: > > MeasureMemory(wait=10) > > else: > > Wait(10) > > sgtm > > I was suggesting keeping the timing of memory measurement as before because I > worried it may trigger a bunch of false alert on memory. I leave it to you to > make the final decision. Yeah, that will almost definitely be the case. Now that you say this, maybe it would indeed be best to re-record everything as you suggested in your email. I'll wait for Juan's opinion on this.
Description was changed from ========== [system-health] Wait for 10 seconds after each story This patch modifies all SH stories to always wait for 10 seconds after loading. In addition, it makes the stories reuse action_runner.MeasureMemory(). BUG=589726,637158 ========== to ========== [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 ==========
Description was changed from ========== [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 ========== to ========== [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 ==========
The CQ bit was checked by petrcermak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. Thanks, Petr
lgtm
On Tue, 16 Aug 2016 at 11:24, Juan Antonio Navarro Pérez < perezju@chromium.org> wrote: > Please do let me have a look before landing this, unless really urgent. I > can review it tomorrow morning (pacific time). > On Tue, 16 Aug 2016 at 11:17, <nednguyen@google.com> wrote: > >> lgtm >> >> >> >> https://codereview.chromium.org/2248693002/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_s5_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build URL) linux_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
lgtm
lgtm but I think that ShouldMeasureMemory is not needed, since the check of whether tracing is enabled already happens within MeasureMemory. 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 This check is already happening within MeasureMemory.
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, charliea@chromium.org, perezju@chromium.org Link to the patchset: https://codereview.chromium.org/2248693002/#ps40001 (title: "Add comment & TODO")
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 ========== [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 ========== to ========== [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 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4cd6d6ceb622b79120215cae46df22f8abe68466 Cr-Commit-Position: refs/heads/master@{#413128}
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. |