Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2307873003:
Add WebView Multiprocess startup benchmark. (Closed)
|
Created:
4 years, 3 months ago by mikecase (-- gone --) Modified:
4 years, 3 months ago CC:
chromium-reviews, hush (inactive), sgurun-gerrit only, telemetry-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd WebView Multiprocess startup benchmark.
This benchmark is almost identical to the existing WebView startup
benchmark, but with the --webview-sandboxed-renderer flag set.
Setting the pageset_repeat option for the WebView start-up benchmarks.
May make metric more stable to smooth out WebView loading from a cold
start vs. WebView being already loaded into Android's memory.
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/47d70e715893705e14ced8862901cf1574bf9a13
Cr-Commit-Position: refs/heads/master@{#417378}
Patch Set 1 #Patch Set 2 : Add WebView Multiprocess startup benchmark. #Patch Set 3 : Add WebView Multiprocess startup benchmark. #
Total comments: 5
Patch Set 4 : pageset_repeat = 20 #Patch Set 5 : Add WebView Multiprocess startup benchmark. #Patch Set 6 : Add WebView Multiprocess startup benchmark. #
Total comments: 1
Patch Set 7 : Added missing newline #
Total comments: 8
Patch Set 8 : Add WebView Multiprocess startup benchmark. #
Messages
Total messages: 23 (8 generated)
Description was changed from ========== Add WebView Multiprocess startup benchmark. This benchmark is almost identical to the existing WebView startup benchmark, but with the --webview-sandboxed-renderer flag set. BUG= ========== to ========== Add WebView Multiprocess startup benchmark. This benchmark is almost identical to the existing WebView startup benchmark, but with the --webview-sandboxed-renderer flag set. BUG= 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 ==========
Description was changed from ========== Add WebView Multiprocess startup benchmark. This benchmark is almost identical to the existing WebView startup benchmark, but with the --webview-sandboxed-renderer flag set. BUG= 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 ========== Add WebView Multiprocess startup benchmark. This benchmark is almost identical to the existing WebView startup benchmark, but with the --webview-sandboxed-renderer flag set. Also, setting the pageset_repeat option. May make metric more stable to smooth out WebView loading from a cold-start vs. being already loaded into Android's memory. BUG= 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 ==========
mikecase@chromium.org changed reviewers: + perezju@chromium.org
https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:94: options = {'pageset_repeat': 10} does the blank page set only contain one blank page? I feel that the repeat of 10 is too few though... https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:111: nit: remove this blank line
perezju@chromium.org changed reviewers: + petrcermak@chromium.org
Please also coordinate with Petr any additions to system_health benchmarks.
Looks good overall, but we should discuss how to incorporate about:blank in the SH story set. It's strange that these benchmarks are called 'system_health.*' even though they don't use the SH story set. Ned: Let's discuss this tomorrow? Please add a bug ID to the "BUG=" line (or remove the line completely). Description nits: 1) "setting the pageset_repeat option" is not a sentence (e.g. "this patch SETS ...") and is unclear. Namely, pageset_repeat of WHAT is set? You should state that you're modifying the existing benchmark as well. In fact, I'd add a note to perf sheriffs in the patch description (example: https://codereview.chromium.org/2203303002/). 2) s/cold-start/cold start/ Thanks, Petr https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:111: On 2016/09/02 18:48:02, hush wrote: > nit: remove this blank line No, don't remove it. There should be two blank lines between all top-level definitions.
Description was changed from ========== Add WebView Multiprocess startup benchmark. This benchmark is almost identical to the existing WebView startup benchmark, but with the --webview-sandboxed-renderer flag set. Also, setting the pageset_repeat option. May make metric more stable to smooth out WebView loading from a cold-start vs. being already loaded into Android's memory. BUG= 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 ========== Add WebView Multiprocess startup benchmark. This benchmark is almost identical to the existing WebView startup benchmark, but with the --webview-sandboxed-renderer flag set. Setting the pageset_repeat option for the WebView start-up benchmarks. May make metric more stable to smooth out WebView loading from a cold start vs. WebView being already loaded into Android's memory. 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 ==========
And updated commit msg. https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:94: options = {'pageset_repeat': 10} On 2016/09/02 at 18:48:02, hush wrote: > does the blank page set only contain one blank page? > > I feel that the repeat of 10 is too few though... Yes, only contains a blank page so the test is very fast. And I up'ed the repeats to 20. https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:111: On 2016/09/05 at 12:45:51, petrcermak wrote: > On 2016/09/02 18:48:02, hush wrote: > > nit: remove this blank line > > No, don't remove it. There should be two blank lines between all top-level definitions. Done
On 2016/09/06 16:40:28, mikecase wrote: > And updated commit msg. > > https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... > File tools/perf/benchmarks/system_health.py (right): > > https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... > tools/perf/benchmarks/system_health.py:94: options = {'pageset_repeat': 10} > On 2016/09/02 at 18:48:02, hush wrote: > > does the blank page set only contain one blank page? > > > > I feel that the repeat of 10 is too few though... > > Yes, only contains a blank page so the test is very fast. And I up'ed the > repeats to 20. > > https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... > tools/perf/benchmarks/system_health.py:111: > On 2016/09/05 at 12:45:51, petrcermak wrote: > > On 2016/09/02 18:48:02, hush wrote: > > > nit: remove this blank line > > > > No, don't remove it. There should be two blank lines between all top-level > definitions. > > Done Chatted offline, "blank:about:blank" is the best name we come up with
On 2016/09/06 at 17:18:50, nednguyen wrote: > On 2016/09/06 16:40:28, mikecase wrote: > > And updated commit msg. > > > > https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... > > File tools/perf/benchmarks/system_health.py (right): > > > > https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... > > tools/perf/benchmarks/system_health.py:94: options = {'pageset_repeat': 10} > > On 2016/09/02 at 18:48:02, hush wrote: > > > does the blank page set only contain one blank page? > > > > > > I feel that the repeat of 10 is too few though... > > > > Yes, only contains a blank page so the test is very fast. And I up'ed the > > repeats to 20. > > > > https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... > > tools/perf/benchmarks/system_health.py:111: > > On 2016/09/05 at 12:45:51, petrcermak wrote: > > > On 2016/09/02 18:48:02, hush wrote: > > > > nit: remove this blank line > > > > > > No, don't remove it. There should be two blank lines between all top-level > > definitions. > > > > Done > > Chatted offline, "blank:about:blank" is the best name we come up with Im not *too* familiar with telemetry. Do you want me to rename something to "blank:about:blank"? Or update the page_sets.BlankPageSet which this uses?
On 2016/09/06 17:22:39, mikecase wrote: > On 2016/09/06 at 17:18:50, nednguyen wrote: > > On 2016/09/06 16:40:28, mikecase wrote: > > > And updated commit msg. > > > > > > > https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... > > > File tools/perf/benchmarks/system_health.py (right): > > > > > > > https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... > > > tools/perf/benchmarks/system_health.py:94: options = {'pageset_repeat': 10} > > > On 2016/09/02 at 18:48:02, hush wrote: > > > > does the blank page set only contain one blank page? > > > > > > > > I feel that the repeat of 10 is too few though... > > > > > > Yes, only contains a blank page so the test is very fast. And I up'ed the > > > repeats to 20. > > > > > > > https://codereview.chromium.org/2307873003/diff/40001/tools/perf/benchmarks/s... > > > tools/perf/benchmarks/system_health.py:111: > > > On 2016/09/05 at 12:45:51, petrcermak wrote: > > > > On 2016/09/02 18:48:02, hush wrote: > > > > > nit: remove this blank line > > > > > > > > No, don't remove it. There should be two blank lines between all top-level > > > definitions. > > > > > > Done > > > > Chatted offline, "blank:about:blank" is the best name we come up with > > Im not *too* familiar with telemetry. Do you want me to rename something to > "blank:about:blank"? > Or update the page_sets.BlankPageSet which this uses? We would like to add about:blank to the SH story set and use that (instead of page_sets.BlankPageSet). To do this: 1. Add a blank_stories.py file to tools/perf/page_sets/system_health with NAME = 'blank:about:blank' (take inspiration from loading_stories.py and browsing_stories.py). 2. Modify the WebView benchmarks to reuse the SH story set with a filter (similarly to what the BattOr benchmarks do: https://cs.chromium.org/chromium/src/tools/perf/benchmarks/battor.py?l=63).
Added system health story BlankStoryAboutBlank that opens about:blank Also added code to wait for at least 1 call to requestAnimationFrame. The BlankPageSet had this so Im guessing it was important. (was added here https://codereview.chromium.org/1475773003) Note that previously the BlankPageSet loaded blank_page.html which I think is a tiny bit different about:blank (looks like blank_page.html is completely empty whereas about:blank at least has an <html> tag. Guessing this doesn't matter) https://codereview.chromium.org/2307873003/diff/100001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/blank_stories.py (right): https://codereview.chromium.org/2307873003/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/blank_stories.py:12: def _DidLoadDocument(self, action_runner): Not 100% sure I need this? For context, this is run for the BlankPageSet and was added in this CL https://codereview.chromium.org/1475773003
non-owner lgtm
Thanks a lot for adding the new story. LGTM with a few more comments. Thanks, Petr https://codereview.chromium.org/2307873003/diff/120001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/blank_stories.py (right): https://codereview.chromium.org/2307873003/diff/120001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/blank_stories.py:8: class _BlankStory(system_health_story.SystemHealthStory): There's no point in splitting the story into two classes. You can just define BlankAboutBlankStory straightaway. https://codereview.chromium.org/2307873003/diff/120001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/blank_stories.py:13: # Request a RAF and wait for it to be processed to ensure that the metric I don't think you need this. The stories wait for 10 seconds if there's no memory measurement and 6 seconds if there are memory measurements. If there hasn't been a RAF for more than 5 seconds, then I think we're really screwed anyway. https://codereview.chromium.org/2307873003/diff/120001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/blank_stories.py:16: """ If you do want to keep this code, than: * s/hasRunRAF/__hasRunRAF/g * s/this/window/g * s/==/===/g * s/0/false/g and s/1/true/g * indent only 2 spaces on line 19 https://codereview.chromium.org/2307873003/diff/120001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/blank_stories.py:26: class BlankStoryAboutBlank(_BlankStory): nit: BlankAboutBlankStory (the last word should be "Story")
https://codereview.chromium.org/2307873003/diff/120001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/blank_stories.py (right): https://codereview.chromium.org/2307873003/diff/120001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/blank_stories.py:8: class _BlankStory(system_health_story.SystemHealthStory): On 2016/09/08 at 11:33:17, petrcermak wrote: > There's no point in splitting the story into two classes. You can just define BlankAboutBlankStory straightaway. Done https://codereview.chromium.org/2307873003/diff/120001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/blank_stories.py:13: # Request a RAF and wait for it to be processed to ensure that the metric On 2016/09/08 at 11:33:17, petrcermak wrote: > I don't think you need this. The stories wait for 10 seconds if there's no memory measurement and 6 seconds if there are memory measurements. If there hasn't been a RAF for more than 5 seconds, then I think we're really screwed anyway. Ack https://codereview.chromium.org/2307873003/diff/120001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/blank_stories.py:16: """ On 2016/09/08 at 11:33:17, petrcermak wrote: > If you do want to keep this code, than: > > * s/hasRunRAF/__hasRunRAF/g > * s/this/window/g > * s/==/===/g > * s/0/false/g and s/1/true/g > * indent only 2 spaces on line 19 Done, Done, Done, Done, Done https://codereview.chromium.org/2307873003/diff/120001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/blank_stories.py:26: class BlankStoryAboutBlank(_BlankStory): On 2016/09/08 at 11:33:17, petrcermak wrote: > nit: BlankAboutBlankStory (the last word should be "Story") Done
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2307873003/#ps140001 (title: "Add WebView Multiprocess startup benchmark.")
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.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add WebView Multiprocess startup benchmark. This benchmark is almost identical to the existing WebView startup benchmark, but with the --webview-sandboxed-renderer flag set. Setting the pageset_repeat option for the WebView start-up benchmarks. May make metric more stable to smooth out WebView loading from a cold start vs. WebView being already loaded into Android's memory. 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 ========== Add WebView Multiprocess startup benchmark. This benchmark is almost identical to the existing WebView startup benchmark, but with the --webview-sandboxed-renderer flag set. Setting the pageset_repeat option for the WebView start-up benchmarks. May make metric more stable to smooth out WebView loading from a cold start vs. WebView being already loaded into Android's memory. 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/47d70e715893705e14ced8862901cf1574bf9a13 Cr-Commit-Position: refs/heads/master@{#417378} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/47d70e715893705e14ced8862901cf1574bf9a13 Cr-Commit-Position: refs/heads/master@{#417378} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
