Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1973953002:
[PCv2] add benchmark page_cycler_v2.typical_25 (Closed)
|
Created:
4 years, 7 months ago by kouhei (in TOK) Modified:
4 years, 7 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[PCv2] add benchmark page_cycler_v2.typical_25
This depends on PCv2 MVP to be landed on catapult repo, like:
https://codereview.chromium.org/1963583005/
BUG=611329
Committed: https://crrev.com/d7f63bfe8e95bfcdf1d1908c6191c6912ab7d808
Cr-Commit-Position: refs/heads/master@{#394420}
Committed: https://crrev.com/6be094854ca6d29a19898686ee25155030a48845
Cr-Commit-Position: refs/heads/master@{#395807}
Patch Set 1 #
Total comments: 6
Patch Set 2 : review #Patch Set 3 : rebased #
Messages
Total messages: 43 (23 generated)
Description was changed from ========== [PCv2] add benchmark page_cycler_v2.typical_25 BUG=611329 ========== to ========== [PCv2] add benchmark page_cycler_v2.typical_25 BUG=611329 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 ==========
Description was changed from ========== [PCv2] add benchmark page_cycler_v2.typical_25 BUG=611329 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 ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 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 ==========
kouhei@chromium.org changed reviewers: + kinuko@chromium.org, nednguyen@google.com, sullivan@chromium.org
nednguyen@google.com changed reviewers: + eakuefner@chromium.org
mostly lgtm. Please wait for Ethan on the issue of naming of cold vs warm pages. https://codereview.chromium.org/1973953002/diff/1/tools/perf/benchmarks/page_... File tools/perf/benchmarks/page_cycler_v2.py (right): https://codereview.chromium.org/1973953002/diff/1/tools/perf/benchmarks/page_... tools/perf/benchmarks/page_cycler_v2.py:20: def Name(cls): There is no need to name this "hidden" benchmark https://codereview.chromium.org/1973953002/diff/1/tools/perf/benchmarks/page_... tools/perf/benchmarks/page_cycler_v2.py:46: cache_temperatures=[cache_temperature.PCV1_COLD, cache_temperature.PCV1_WARM]) nits: this should go to a new line. If the PRESUBMIT didn't warn you, let me know https://codereview.chromium.org/1973953002/diff/1/tools/perf/page_sets/typica... File tools/perf/page_sets/typical_25.py (right): https://codereview.chromium.org/1973953002/diff/1/tools/perf/page_sets/typica... tools/perf/page_sets/typical_25.py:115: url, self, run_no_page_interactions, cache_temperature=temp)) I don't remember whether we concatenate the cache temperature to the page name. If not, you may need to explicitly name the page with cache_temperature as part of the name or telemetry will merge this pages with same name.
lgtm https://codereview.chromium.org/1973953002/diff/1/tools/perf/page_sets/typica... File tools/perf/page_sets/typical_25.py (right): https://codereview.chromium.org/1973953002/diff/1/tools/perf/page_sets/typica... tools/perf/page_sets/typical_25.py:115: url, self, run_no_page_interactions, cache_temperature=temp)) On 2016/05/13 at 03:43:22, nednguyen wrote: > I don't remember whether we concatenate the cache temperature to the page name. If not, you may need to explicitly name the page with cache_temperature as part of the name or telemetry will merge this pages with same name. When PageTestResults.AddValue is called, all the story keys will get concatenated to the value name. So in this case, the value from the warm page and the value from the cold page will have different names, so Telemetry will never attempt to merge. I think this is fine.
Thanks for your reviews! I just found yesterday with the current PCv2 implementation that it records slow ttfcp on first nav due to disk cache async init. This causes the PCv2 results to be noisier than intended. I'd like to land this CL after fixing the issue. On 2016/05/16 18:27:13, eakuefner wrote: > lgtm > > https://codereview.chromium.org/1973953002/diff/1/tools/perf/page_sets/typica... > File tools/perf/page_sets/typical_25.py (right): > > https://codereview.chromium.org/1973953002/diff/1/tools/perf/page_sets/typica... > tools/perf/page_sets/typical_25.py:115: url, self, run_no_page_interactions, > cache_temperature=temp)) > On 2016/05/13 at 03:43:22, nednguyen wrote: > > I don't remember whether we concatenate the cache temperature to the page > name. If not, you may need to explicitly name the page with cache_temperature as > part of the name or telemetry will merge this pages with same name. > > When PageTestResults.AddValue is called, all the story keys will get > concatenated to the value name. So in this case, the value from the warm page > and the value from the cold page will have different names, so Telemetry will > never attempt to merge. I think this is fine. Thanks for confirming!
https://codereview.chromium.org/1973953002/diff/1/tools/perf/benchmarks/page_... File tools/perf/benchmarks/page_cycler_v2.py (right): https://codereview.chromium.org/1973953002/diff/1/tools/perf/benchmarks/page_... tools/perf/benchmarks/page_cycler_v2.py:20: def Name(cls): On 2016/05/13 03:43:22, nednguyen wrote: > There is no need to name this "hidden" benchmark Done. https://codereview.chromium.org/1973953002/diff/1/tools/perf/benchmarks/page_... tools/perf/benchmarks/page_cycler_v2.py:46: cache_temperatures=[cache_temperature.PCV1_COLD, cache_temperature.PCV1_WARM]) On 2016/05/13 03:43:22, nednguyen wrote: > nits: this should go to a new line. If the PRESUBMIT didn't warn you, let me > know Done.
The CQ bit was checked by kouhei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1973953002/#ps20001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973953002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973953002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973953002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973953002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_perf_cq on tryserver.chromium.perf (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_cq/bu...)
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973953002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973953002/20001
Description was changed from ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 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 ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
The CQ bit was unchecked by nednguyen@google.com
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973953002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973953002/20001
Message was sent while issue was closed.
Description was changed from ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq Committed: https://crrev.com/d7f63bfe8e95bfcdf1d1908c6191c6912ab7d808 Cr-Commit-Position: refs/heads/master@{#394420} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d7f63bfe8e95bfcdf1d1908c6191c6912ab7d808 Cr-Commit-Position: refs/heads/master@{#394420}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1996553002/ by skyostil@chromium.org. The reason for reverting is: This new benchmark seems to be failing on Android, Mac and Windows: https://build.chromium.org/p/chromium.perf/builders/Android%20Nexus5X%20Perf%... https://build.chromium.org/p/chromium.perf/builders/Win%208%20Perf%20%282%29/... https://build.chromium.org/p/chromium.perf/builders/Mac%2010.11%20Perf%20%282....
Message was sent while issue was closed.
Description was changed from ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq Committed: https://crrev.com/d7f63bfe8e95bfcdf1d1908c6191c6912ab7d808 Cr-Commit-Position: refs/heads/master@{#394420} ========== to ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq Committed: https://crrev.com/d7f63bfe8e95bfcdf1d1908c6191c6912ab7d808 Cr-Commit-Position: refs/heads/master@{#394420} ==========
On 2016/05/19 11:15:08, Sami wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/1996553002/ by mailto:skyostil@chromium.org. > > The reason for reverting is: This new benchmark seems to be failing on Android, > Mac and Windows: > > https://build.chromium.org/p/chromium.perf/builders/Android%20Nexus5X%20Perf%... > > https://build.chromium.org/p/chromium.perf/builders/Win%208%20Perf%20%282%29/... > > https://build.chromium.org/p/chromium.perf/builders/Mac%2010.11%20Perf%20%282.... We fixed this issue in https://codereview.chromium.org/1988313003/ Ned: Can we try relanding this now?
The CQ bit was checked by kouhei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1973953002/#ps40001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973953002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973953002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_retina_perf_cq on tryserver.chromium.perf (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf_...)
Description was changed from ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq Committed: https://crrev.com/d7f63bfe8e95bfcdf1d1908c6191c6912ab7d808 Cr-Commit-Position: refs/heads/master@{#394420} ========== to ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 Committed: https://crrev.com/d7f63bfe8e95bfcdf1d1908c6191c6912ab7d808 Cr-Commit-Position: refs/heads/master@{#394420} ==========
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973953002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973953002/40001
Message was sent while issue was closed.
Description was changed from ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 Committed: https://crrev.com/d7f63bfe8e95bfcdf1d1908c6191c6912ab7d808 Cr-Commit-Position: refs/heads/master@{#394420} ========== to ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 Committed: https://crrev.com/d7f63bfe8e95bfcdf1d1908c6191c6912ab7d808 Cr-Commit-Position: refs/heads/master@{#394420} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 Committed: https://crrev.com/d7f63bfe8e95bfcdf1d1908c6191c6912ab7d808 Cr-Commit-Position: refs/heads/master@{#394420} ========== to ========== [PCv2] add benchmark page_cycler_v2.typical_25 This depends on PCv2 MVP to be landed on catapult repo, like: https://codereview.chromium.org/1963583005/ BUG=611329 Committed: https://crrev.com/d7f63bfe8e95bfcdf1d1908c6191c6912ab7d808 Cr-Commit-Position: refs/heads/master@{#394420} Committed: https://crrev.com/6be094854ca6d29a19898686ee25155030a48845 Cr-Commit-Position: refs/heads/master@{#395807} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6be094854ca6d29a19898686ee25155030a48845 Cr-Commit-Position: refs/heads/master@{#395807} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
