Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1802273002:
[telemetry] Add a version of v8.todomvc that runs using ignition (Closed)
|
Created:
4 years, 9 months ago by jochen (gone - plz use gerrit) Modified:
4 years, 9 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[telemetry] Add a version of v8.todomvc that runs using ignition
R=rmcilroy@chromium.org,cbruni@chromium.org,danno@chromium.org,skyostil@chromium.org
BUG=chromium:593406, v8:4280
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect
Committed: https://crrev.com/52ad53088c557f997dbb969e30902bbd5ffe5dd7
Cr-Commit-Position: refs/heads/master@{#381716}
Patch Set 1 #Patch Set 2 : updates #
Total comments: 4
Messages
Total messages: 20 (7 generated)
Description was changed from ========== [telemetry] Add a version of v8.todomvc that runs using ignition R=rmcilroy@chromium.org,cbruni@chromium.org,danno@chromium.org,skyostil@chrom... BUG=chromium:593406,v8:4280 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect ========== to ========== [telemetry] Add a version of v8.todomvc that runs using ignition R=rmcilroy@chromium.org,cbruni@chromium.org,danno@chromium.org,skyostil@chrom... BUG=chromium:593406,v8:4280 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect ==========
skyostil@chromium.org changed reviewers: + zhenw@chromium.org
https://codereview.chromium.org/1802273002/diff/20001/tools/perf/benchmarks/v... File tools/perf/benchmarks/v8.py (right): https://codereview.chromium.org/1802273002/diff/20001/tools/perf/benchmarks/v... tools/perf/benchmarks/v8.py:169: def ShouldTearDownStateAfterEachStoryRun(cls): Like we discussed this should probably use the deprecated-but-working restart API (i.e., needs_browser_restart_after_each_page=True) since https://crrev.com/43f84917812c614b2a38e8e27064c6873ca6cdb0 got reverted. MemoryInfra also has this bug (crbug.com/594952). Zhen, can you suggest a way to use the old API from here?
https://codereview.chromium.org/1802273002/diff/20001/tools/perf/benchmarks/v... File tools/perf/benchmarks/v8.py (right): https://codereview.chromium.org/1802273002/diff/20001/tools/perf/benchmarks/v... tools/perf/benchmarks/v8.py:169: def ShouldTearDownStateAfterEachStoryRun(cls): On 2016/03/15 at 12:13:55, Sami wrote: > Like we discussed this should probably use the deprecated-but-working restart API (i.e., needs_browser_restart_after_each_page=True) since https://crrev.com/43f84917812c614b2a38e8e27064c6873ca6cdb0 got reverted. MemoryInfra also has this bug (crbug.com/594952). > > Zhen, can you suggest a way to use the old API from here? needs_browser_restart_after_each_page only works with pre-timeline based tests. For timeline based measurements, you'd have to specify a startup-url (which works), but then timeline based measurements crashes, because it starts the tracing controller before the story is run, and expects it to stay active (which it doesn't when the entire browser is swapped).
lgtm
https://codereview.chromium.org/1802273002/diff/20001/tools/perf/benchmarks/v... File tools/perf/benchmarks/v8.py (right): https://codereview.chromium.org/1802273002/diff/20001/tools/perf/benchmarks/v... tools/perf/benchmarks/v8.py:169: def ShouldTearDownStateAfterEachStoryRun(cls): On 2016/03/15 12:19:08, jochen wrote: > On 2016/03/15 at 12:13:55, Sami wrote: > > Like we discussed this should probably use the deprecated-but-working restart > API (i.e., needs_browser_restart_after_each_page=True) since > https://crrev.com/43f84917812c614b2a38e8e27064c6873ca6cdb0 got reverted. > MemoryInfra also has this bug (crbug.com/594952). > > > > Zhen, can you suggest a way to use the old API from here? > > needs_browser_restart_after_each_page only works with pre-timeline based tests. > > For timeline based measurements, you'd have to specify a startup-url (which > works), but then timeline based measurements crashes, because it starts the > tracing controller before the story is run, and expects it to stay active (which > it doesn't when the entire browser is swapped). This API should not be used. What are the use cases here? Please continue the discussion in https://bugs.chromium.org/p/chromium/issues/detail?id=594952. Thanks for finding this. :)
On 2016/03/15 at 15:48:44, zhenw wrote: > https://codereview.chromium.org/1802273002/diff/20001/tools/perf/benchmarks/v... > File tools/perf/benchmarks/v8.py (right): > > https://codereview.chromium.org/1802273002/diff/20001/tools/perf/benchmarks/v... > tools/perf/benchmarks/v8.py:169: def ShouldTearDownStateAfterEachStoryRun(cls): > On 2016/03/15 12:19:08, jochen wrote: > > On 2016/03/15 at 12:13:55, Sami wrote: > > > Like we discussed this should probably use the deprecated-but-working restart > > API (i.e., needs_browser_restart_after_each_page=True) since > > https://crrev.com/43f84917812c614b2a38e8e27064c6873ca6cdb0 got reverted. > > MemoryInfra also has this bug (crbug.com/594952). > > > > > > Zhen, can you suggest a way to use the old API from here? > > > > needs_browser_restart_after_each_page only works with pre-timeline based tests. > > > > For timeline based measurements, you'd have to specify a startup-url (which > > works), but then timeline based measurements crashes, because it starts the > > tracing controller before the story is run, and expects it to stay active (which > > it doesn't when the entire browser is swapped). > > This API should not be used. What are the use cases here? Please continue the discussion in https://bugs.chromium.org/p/chromium/issues/detail?id=594952. Thanks for finding this. :) now that the API was added back to telemetry, PTAL :)
lgtm now that ShouldTearDownStateAfterEachStoryRun works again.
The CQ bit was checked by jochen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1802273002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802273002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_10_10_perf_bisect on tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
LGTM, but we won't get compile time metrics yet I don't think (see below). https://codereview.chromium.org/1802273002/diff/20001/tools/perf/benchmarks/v... File tools/perf/benchmarks/v8.py (right): https://codereview.chromium.org/1802273002/diff/20001/tools/perf/benchmarks/v... tools/perf/benchmarks/v8.py:161: options.SetLegacyTimelineBasedMetrics([v8_execution.V8ExecutionMetric()]) I'm not sure these will capture any compile time for ignition since they look for V8.CompileFullCode explicitly. I'll land an equivalent trace event for ignition, but we'll need to update these metrics too before we get any results.
Description was changed from ========== [telemetry] Add a version of v8.todomvc that runs using ignition R=rmcilroy@chromium.org,cbruni@chromium.org,danno@chromium.org,skyostil@chrom... BUG=chromium:593406,v8:4280 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect ========== to ========== [telemetry] Add a version of v8.todomvc that runs using ignition R=rmcilroy@chromium.org,cbruni@chromium.org,danno@chromium.org,skyostil@chrom... BUG=chromium:593406,v8:4280 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect ==========
The CQ bit was checked by jochen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1802273002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802273002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [telemetry] Add a version of v8.todomvc that runs using ignition R=rmcilroy@chromium.org,cbruni@chromium.org,danno@chromium.org,skyostil@chrom... BUG=chromium:593406,v8:4280 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect ========== to ========== [telemetry] Add a version of v8.todomvc that runs using ignition R=rmcilroy@chromium.org,cbruni@chromium.org,danno@chromium.org,skyostil@chrom... BUG=chromium:593406,v8:4280 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect Committed: https://crrev.com/52ad53088c557f997dbb969e30902bbd5ffe5dd7 Cr-Commit-Position: refs/heads/master@{#381716} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/52ad53088c557f997dbb969e30902bbd5ffe5dd7 Cr-Commit-Position: refs/heads/master@{#381716} |
