Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
962783002:
Adding a page cycler becnchmark for measuring out-of-process iframes. (Closed)
|
Created:
5 years, 10 months ago by nasko Modified:
5 years, 9 months ago Reviewers:
sullivan CC:
chromium-reviews, telemetry-reviews_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding a page cycler becnchmark for measuring out-of-process iframes.
BUG=462323
Committed: https://crrev.com/e263b52301b6374484ad0a14b013be356ee29b84
Cr-Commit-Position: refs/heads/master@{#318732}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Move oopif benchmark over to the main page_cycler file. #Messages
Total messages: 16 (4 generated)
Patchset #1 (id:1) has been deleted
nasko@chromium.org changed reviewers: + tonyg@chromium.org
Hi Tony, We've discussed in the past adding a perf benchmark for out-of-process iframes. I've finally gotten around to getting this work done and this is the first CL to get us started. Can you review it? Thanks in advance! Nasko
One thing to note - in my local testing it doesn't seem to always go through all of the 25 test cases for some reason. In addition, on some of the loads we non-deterministically time out. Would this be a problem for the perf infrastructure or does it handle properly having a benchmark that fails on some runs?
Hey nasko, could you please pick a currently active reviewer for this dir? $ cat ./tools/perf/OWNERS set noparent achuith@chromium.org dtu@chromium.org epenner@chromium.org nednguyen@google.com skyostil@chromium.org sullivan@chromium.org ...
nasko@chromium.org changed reviewers: + sullivan@chromium.org - tonyg@chromium.org
Huh, one of the extensions that helps with reviewers put your name in the list. Sorry, replacing with sullivan@.
https://codereview.chromium.org/962783002/diff/20001/tools/perf/benchmarks/pa... File tools/perf/benchmarks/page_cycler_oopif.py (right): https://codereview.chromium.org/962783002/diff/20001/tools/perf/benchmarks/pa... tools/perf/benchmarks/page_cycler_oopif.py:26: options.AppendExtraBrowserArgs(['--site-per-process']) I'd prefer to just move PageCyclerOopifTypical25 to page_cycler.py, make it sublclass the _PageCycler class there, and just override CustomizeBrowserOptions in the PageCyclerOopifTypical25 class. That way: - It's clearer what the difference between this and other page cyclers is - If anything changes in the other _PageCycler class, don't have to copy/paste it here. https://codereview.chromium.org/962783002/diff/20001/tools/perf/benchmarks/pa... tools/perf/benchmarks/page_cycler_oopif.py:37: @benchmark.Disabled('win') Can use @benchmark.Disabled('xp') if that is a better fit. But it looks like after this comment was written in page_cycler.py, there were timeouts on our win7 perfbots, too. So maybe "flakey on windows"? (If you're moving this to page_cycler.py, please update the comment you copy/pasted from as well!)
Thanks for the suggestions. All implemented. https://codereview.chromium.org/962783002/diff/20001/tools/perf/benchmarks/pa... File tools/perf/benchmarks/page_cycler_oopif.py (right): https://codereview.chromium.org/962783002/diff/20001/tools/perf/benchmarks/pa... tools/perf/benchmarks/page_cycler_oopif.py:26: options.AppendExtraBrowserArgs(['--site-per-process']) On 2015/02/28 15:25:46, sullivan wrote: > I'd prefer to just move PageCyclerOopifTypical25 to page_cycler.py, make it > sublclass the _PageCycler class there, and just override CustomizeBrowserOptions > in the PageCyclerOopifTypical25 class. That way: > > - It's clearer what the difference between this and other page cyclers is > - If anything changes in the other _PageCycler class, don't have to copy/paste > it here. Done. https://codereview.chromium.org/962783002/diff/20001/tools/perf/benchmarks/pa... tools/perf/benchmarks/page_cycler_oopif.py:37: @benchmark.Disabled('win') On 2015/02/28 15:25:46, sullivan wrote: > Can use @benchmark.Disabled('xp') if that is a better fit. But it looks like > after this comment was written in page_cycler.py, there were timeouts on our > win7 perfbots, too. So maybe "flakey on windows"? (If you're moving this to > page_cycler.py, please update the comment you copy/pasted from as well!) Done.
lgtm
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962783002/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e263b52301b6374484ad0a14b013be356ee29b84 Cr-Commit-Position: refs/heads/master@{#318732}
Message was sent while issue was closed.
Hay <perf sheriff> This new benchmark is crashing Chrome on all platforms, both ToT and reference build. I'm going to disable it for now. I filed a bug: https://code.google.com/p/chromium/issues/detail?id=463346 Here's stacks on Linux and Mac: http://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%284%29/bui... http://build.chromium.org/p/chromium.perf/builders/Mac%2010.9%20Perf%20%284%2...
Message was sent while issue was closed.
Oh! Also: <owner review> I think we want to use module.Class naming for new benchmarks. (That is, the name generated if you don't override the Name() method.) |
