Clean up memory benchmark
- Only access browser via actions instead of directly using the
selenium driver
- Add Browser.switch_window and Browser.window_id APIs
- Use double quotes
- Move ActionRunnerListener to separate files to limit imports
- Rename WebdriverBrowser._driver to _private_driver as it should no
longer be accessed directly from the outside
Change-Id: I9479b4ed672a923bd90fc2034f9b4e62c92c71cd
Reviewed-on: https://chromium-review.googlesource.com/c/crossbench/+/5913677
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Suhua Lei <lsuhua@google.com>
diff --git a/crossbench/benchmarks/loading/action_runner/action_runner_listener.py b/crossbench/benchmarks/loading/action_runner/action_runner_listener.py
new file mode 100644
index 0000000..b77bef3
--- /dev/null
+++ b/crossbench/benchmarks/loading/action_runner/action_runner_listener.py
@@ -0,0 +1,23 @@
+# Copyright 2024 The Chromium Authors
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from crossbench.runner.run import Run
+
+
+class ActionRunnerListener:
+ """Default empty ActionRunnerListener implementation."""
+
+ def handle_error(self, e: Exception) -> None:
+ pass
+
+ def handle_page_run(self, run: Run) -> None:
+ pass
+
+ def handle_new_tab(self) -> None:
+ pass
\ No newline at end of file
diff --git a/crossbench/benchmarks/loading/action_runner/base.py b/crossbench/benchmarks/loading/action_runner/base.py
index 91640a4..da16268 100644
--- a/crossbench/benchmarks/loading/action_runner/base.py
+++ b/crossbench/benchmarks/loading/action_runner/base.py
@@ -7,6 +7,8 @@
from typing import TYPE_CHECKING, Iterable, Optional
from crossbench import exception
+from crossbench.benchmarks.loading.action_runner.action_runner_listener import \
+ ActionRunnerListener
from crossbench.benchmarks.loading.input_source import InputSource
if TYPE_CHECKING:
@@ -53,18 +55,6 @@
super().__init__(runner, action, input_source_message)
-class ActionRunnerListener:
- """Default empty ActionRunnerListener implementation."""
-
- def handle_error(self, e: Exception) -> None:
- pass
-
- def handle_page_run(self, run: Run) -> None:
- pass
-
- def handle_new_tab(self) -> None:
- pass
-
class ActionRunner:
diff --git a/crossbench/benchmarks/memory/memory_benchmark.py b/crossbench/benchmarks/memory/memory_benchmark.py
index 8df02b2..0864503 100644
--- a/crossbench/benchmarks/memory/memory_benchmark.py
+++ b/crossbench/benchmarks/memory/memory_benchmark.py
@@ -7,28 +7,26 @@
import datetime as dt
import logging
import time
-
-from typing import TYPE_CHECKING, Any, Dict, Optional, Sequence, Type, Tuple
+from typing import TYPE_CHECKING, Any, Dict, Optional, Sequence, Tuple, Type
import selenium.common.exceptions
import urllib3.exceptions
-from selenium import webdriver
from crossbench import helper
from crossbench.benchmarks.base import StoryFilter, SubStoryBenchmark
-from crossbench.benchmarks.loading.action_runner.base import (
- ActionRunner, ActionRunnerListener)
+from crossbench.benchmarks.loading.action_runner.action_runner_listener import \
+ ActionRunnerListener
from crossbench.benchmarks.loading.action_runner.basic_action_runner import \
BasicActionRunner
from crossbench.benchmarks.loading.page import LivePage, Page
from crossbench.benchmarks.loading.tab_controller import TabController
-from crossbench.browsers.webdriver import WebDriverBrowser
from crossbench.parse import NumberParser
from crossbench.runner.exception import StopStoryException
if TYPE_CHECKING:
import argparse
+ from crossbench.benchmarks.loading.action_runner.base import ActionRunner
from crossbench.cli.parser import CrossBenchArgumentParser
from crossbench.runner.run import Run
@@ -48,22 +46,22 @@
cls, parser: argparse.ArgumentParser) -> argparse.ArgumentParser:
parser = super().add_cli_parser(parser)
parser.add_argument(
- '--alloc-count',
+ "--alloc-count",
type=NumberParser.positive_int,
default=1,
- help='The number of block to allocate.')
+ help="The number of block to allocate.")
parser.add_argument(
- '--block-size',
+ "--block-size",
type=NumberParser.positive_int,
default=128,
- help='The size of each block (MB).')
+ help="The size of each block (MB).")
parser.add_argument(
- '--compressibility',
+ "--compressibility",
type=NumberParser.positive_zero_int,
default=0,
- help='The compressibility (0-100)')
+ help="The compressibility (0-100)")
parser.add_argument(
- '--prefill-constant',
+ "--prefill-constant",
type=NumberParser.any_int,
default=1,
help="Prefill memory buffer with given constant (-1-127)."
@@ -99,7 +97,7 @@
dest="tabs",
const=TabController.forever(),
action="store_const",
- help="Open memory workload in seperate tabs infinitely."
+ help="Open memory workload in separate tabs infinitely."
"Equivalent to --tabs=infinity")
return parser
@@ -159,11 +157,10 @@
parser = super().add_cli_parser(subparsers, aliases)
cls.STORY_FILTER_CLS.add_cli_parser(parser)
parser.add_argument(
- '--skippable-tab-count',
- dest="skippable_tab_count",
+ "--skippable-tab-count",
type=NumberParser.positive_int,
default=0,
- help='The number of tabs that can be skipped for liveness checking.')
+ help="The number of tabs that can be skipped for liveness checking.")
return parser
@classmethod
@@ -184,16 +181,16 @@
def __init__(self,
stories: Sequence[Page],
- skippable_tab_count: Optional[int] = 0,
+ skippable_tab_count: int = 0,
action_runner: Optional[ActionRunner] = None) -> None:
self._action_runner = action_runner or BasicActionRunner()
for story in stories:
assert isinstance(story, Page)
super().__init__(stories)
- # Records the navigation_starttime time for each window handle.
- self.navigation_time_ms: Dict[str, float] = {}
- self.tab_count: int = 1
- self.skippable_tab_count = skippable_tab_count
+ # Records the navigation_start_time time for each window handle.
+ self._navigation_time_ms: Dict[str, float] = {}
+ self._tab_count: int = 1
+ self._skippable_tab_count = skippable_tab_count
self._action_runner.set_listener(self)
@classmethod
@@ -206,26 +203,20 @@
def action_runner(self) -> ActionRunner:
return self._action_runner
- def get_driver(self, run: Run) -> webdriver.Remote:
- if isinstance(run.browser, WebDriverBrowser):
- return run.browser.driver
- raise TypeError("Memory benchmark only supports WebDriverBrowser.")
-
def _increment_tab_count(self):
- self.tab_count += 1
+ self._tab_count += 1
def _record_navigation_time(self, run: Run) -> None:
"""
Record NavigationStart time for each handle.
"""
- driver = self.get_driver(run)
- cur_handle: str = driver.current_window_handle
-
- navigation_starttime = driver.execute_script(
- "return window.performance.timing.navigationStart")
- logging.debug("Navigation starttime for handle %s is %s.", cur_handle,
- navigation_starttime)
- self.navigation_time_ms[cur_handle] = navigation_starttime
+ with run.actions("_record_navigation_time", measure=False) as action:
+ cur_handle: str = action.current_window_id()
+ navigation_start_time = action.js(
+ "return window.performance.timing.navigationStart")
+ logging.debug("Navigation starttime for handle %s is %s.", cur_handle,
+ navigation_start_time)
+ self._navigation_time_ms[cur_handle] = navigation_start_time
def _check_liveness(self, run: Run) -> None:
"""
@@ -233,19 +224,19 @@
has changed. If so, then it means that page has been discarded
and reloaded.
"""
- driver = self.get_driver(run)
- for handle in self.navigation_time_ms:
- logging.debug("Liveness checking for handle: %s", handle)
- driver.switch_to.window(handle)
- time.sleep(1)
- navigation_starttime = driver.execute_script(
- "return window.performance.timing.navigationStart")
-
- if navigation_starttime != self.navigation_time_ms[handle]:
- logging.info(
- "The max num of tabs we can keep alive concurrently is: %s ",
- self.tab_count - 1)
- raise StopStoryException("Found a page that has been reloaded.")
+ with run.actions("_check_liveness", measure=False) as action:
+ for handle in self._navigation_time_ms:
+ logging.debug("Liveness checking for handle: %s", handle)
+ action.switch_window(handle)
+ action.wait(1)
+ navigation_start_time = action.js(
+ "return window.performance.timing.navigationStart")
+ if navigation_start_time != self._navigation_time_ms[handle]:
+ logging.debug("Found a page that has been reloaded!")
+ logging.info(
+ "The max num of tabs we can keep alive concurrently is: %s ",
+ self._tab_count - 1)
+ raise StopStoryException("Found a page that has been reloaded.")
def handle_error(self, e: Exception) -> None:
"""
@@ -257,12 +248,12 @@
) and "page crash" in str(e) or isinstance(
e, urllib3.exceptions.ReadTimeoutError):
logging.info("The max num of tabs we can keep alive concurrently is: %s ",
- self.tab_count - 1)
+ self._tab_count - 1)
raise StopStoryException(f"Found a Tab Crash/Timeout: {e}")
def handle_page_run(self, run: Run) -> None:
self._record_navigation_time(run)
- if self.tab_count > self.skippable_tab_count:
+ if self._tab_count > self._skippable_tab_count:
self._check_liveness(run)
def handle_new_tab(self) -> None:
diff --git a/crossbench/browsers/browser.py b/crossbench/browsers/browser.py
index 0de50ef..e1f8721 100644
--- a/crossbench/browsers/browser.py
+++ b/crossbench/browsers/browser.py
@@ -319,6 +319,13 @@
raise NotImplementedError(
f"New document script injection is not supported by {self}")
+ def current_window_id(self) -> str:
+ raise NotImplementedError(f"current_window_id is not implemented by {self}")
+
+ def switch_window(self, window_id: str) -> None:
+ del window_id
+ raise NotImplementedError(f"switch_window is not implemented by {self}")
+
def switch_tab(
self,
title: Optional[re.Pattern] = None,
diff --git a/crossbench/browsers/chromium/webdriver.py b/crossbench/browsers/chromium/webdriver.py
index 64f43e5..92a0121 100644
--- a/crossbench/browsers/chromium/webdriver.py
+++ b/crossbench/browsers/chromium/webdriver.py
@@ -25,8 +25,9 @@
from selenium.webdriver.chromium.webdriver import ChromiumDriver
from selenium.webdriver.remote.webdriver import WebDriver as RemoteWebDriver
-from crossbench import exception, helper, plt
+from crossbench import exception, helper
from crossbench import path as pth
+from crossbench import plt
from crossbench.browsers.attributes import BrowserAttributes
from crossbench.browsers.browser_helper import BROWSERS_CACHE
from crossbench.browsers.chromium.chromium import Chromium
@@ -167,8 +168,14 @@
f"browser={self.version} ({self})",)
def run_script_on_new_document(self, script: str) -> None:
- self._driver.execute_cdp_cmd("Page.addScriptToEvaluateOnNewDocument",
- {"source": script})
+ self._private_driver.execute_cdp_cmd(
+ "Page.addScriptToEvaluateOnNewDocument", {"source": script})
+
+ def current_window_id(self) -> str:
+ return str(self._private_driver.current_window_handle)
+
+ def switch_window(self, window_id: str) -> None:
+ self._private_driver.switch_to.window(window_id)
def switch_tab(
self,
@@ -177,7 +184,7 @@
tab_index: Optional[int] = None,
timeout: dt.timedelta = dt.timedelta(seconds=0)
) -> None:
- driver = self._driver
+ driver = self._private_driver
original_handle = driver.current_window_handle
for _ in helper.wait_with_backoff(timeout, self.platform):
# Search through other handles starting from current_window_handle + 1
@@ -210,9 +217,9 @@
raise RuntimeError(error)
def start_profiling(self) -> None:
- assert isinstance(self._driver, ChromiumDriver)
+ assert isinstance(self._private_driver, ChromiumDriver)
# TODO: reuse the TraceProbe categories,
- self._driver.execute_cdp_cmd(
+ self._private_driver.execute_cdp_cmd(
"Tracing.start", {
"transferMode":
"ReturnAsStream",
@@ -231,8 +238,8 @@
})
def stop_profiling(self) -> Any:
- assert isinstance(self._driver, ChromiumDriver)
- data = self._driver.execute_cdp_cmd("Tracing.tracingComplete", {})
+ assert isinstance(self._private_driver, ChromiumDriver)
+ data = self._private_driver.execute_cdp_cmd("Tracing.tracingComplete", {})
# TODO: use webdriver bidi to get the async Tracing.end event.
# self._driver.execute_cdp_cmd("Tracing.end", {})
return data
diff --git a/crossbench/browsers/safari/webdriver.py b/crossbench/browsers/safari/webdriver.py
index 3af1562..d2f2668 100644
--- a/crossbench/browsers/safari/webdriver.py
+++ b/crossbench/browsers/safari/webdriver.py
@@ -192,7 +192,7 @@
pass
def quit(self) -> None:
- self._driver.close()
+ self._private_driver.close()
self.platform.sleep(1.0)
- self._driver.quit()
+ self._private_driver.quit()
self.force_quit()
diff --git a/crossbench/browsers/webdriver.py b/crossbench/browsers/webdriver.py
index 16df64d..57481f7 100644
--- a/crossbench/browsers/webdriver.py
+++ b/crossbench/browsers/webdriver.py
@@ -49,7 +49,7 @@
class WebDriverBrowser(Browser, metaclass=abc.ABCMeta):
- _driver: webdriver.Remote
+ _private_driver: webdriver.Remote
_driver_path: Optional[AnyPath]
_driver_pid: int
_pid: int
@@ -67,10 +67,6 @@
return BrowserAttributes.WEBDRIVER
@property
- def driver(self) -> webdriver.Remote:
- return self._driver
-
- @property
def driver_log_file(self) -> LocalPath:
log_file = self.log_file
assert log_file
@@ -100,7 +96,7 @@
logging.debug("Setting http request timeout to %s", timeout)
RemoteConnection.set_timeout(timeout.total_seconds())
try:
- self._driver = self._start_driver(session, self._driver_path)
+ self._private_driver = self._start_driver(session, self._driver_path)
except selenium.common.exceptions.SessionNotCreatedException as e:
msg = e.msg or "Could not create Webdriver session."
raise DriverException(msg, self) from e
@@ -111,7 +107,7 @@
self._setup_window()
def _find_driver_pid(self) -> None:
- service = getattr(self._driver, "service", None)
+ service = getattr(self._private_driver, "service", None)
if not service:
return
self._driver_pid = service.process.pid
@@ -139,7 +135,7 @@
factor = timing.timeout_unit.total_seconds()
if factor != 1.0:
logging.info("Increasing webdriver timeouts by %fx", factor)
- timeouts: Timeouts = self.driver.timeouts
+ timeouts: Timeouts = self._private_driver.timeouts
if implicit_wait := getattr(timeouts, "implicit_wait", None):
timeouts.implicit_wait = timing.timeout_timedelta(
implicit_wait).total_seconds()
@@ -147,20 +143,22 @@
timeouts.script = timing.timeout_timedelta(script).total_seconds()
if page_load := getattr(timeouts, "page_load", None):
timeouts.page_load = timing.timeout_timedelta(page_load).total_seconds()
- self.driver.timeouts = timeouts
+ self._private_driver.timeouts = timeouts
def _setup_window(self) -> None:
# Force main window to foreground.
- self._driver.switch_to.window(self._driver.current_window_handle)
+ self._private_driver.switch_to.window(
+ self._private_driver.current_window_handle)
if self.viewport.is_headless:
return
if self.viewport.is_fullscreen:
- self._driver.fullscreen_window()
+ self._private_driver.fullscreen_window()
elif self.viewport.is_maximized:
- self._driver.maximize_window()
+ self._private_driver.maximize_window()
else:
- self._driver.set_window_position(self.viewport.x, self.viewport.y)
- self._driver.set_window_size(self.viewport.width, self.viewport.height)
+ self._private_driver.set_window_position(self.viewport.x, self.viewport.y)
+ self._private_driver.set_window_size(self.viewport.width,
+ self.viewport.height)
@abc.abstractmethod
def _start_driver(self, session: BrowserSessionRunGroup,
@@ -178,26 +176,26 @@
logging.debug("WebDriverBrowser.show_url(%s, %s)", url, target)
try:
if target in ("_self", None):
- handles = self._driver.window_handles
+ handles = self._private_driver.window_handles
assert handles, "Browser has no more opened windows."
- self._driver.switch_to.window(handles[-1])
+ self._private_driver.switch_to.window(handles[-1])
elif target == "_new_tab":
- self._driver.switch_to.new_window("tab")
+ self._private_driver.switch_to.new_window("tab")
elif target == "_new_window":
- self._driver.switch_to.new_window("window")
+ self._private_driver.switch_to.new_window("window")
else:
raise RuntimeError(f"unexpected target {target}")
- self._driver.get(url)
+ self._private_driver.get(url)
except selenium.common.exceptions.WebDriverException as e:
if msg := e.msg:
self._wrap_webdriver_exception(e, msg, url)
raise
def switch_to_new_tab(self) -> None:
- self._driver.switch_to.new_window("tab")
+ self._private_driver.switch_to.new_window("tab")
def screenshot(self, path: LocalPath) -> None:
- if not self._driver.get_screenshot_as_file(path.as_posix()):
+ if not self._private_driver.get_screenshot_as_file(path.as_posix()):
raise DriverException(
f"Browser failed to get_screenshot_as_file to file '{path}'", self)
@@ -226,18 +224,18 @@
if timeout is not None:
assert timeout.total_seconds() > 0, (
f"timeout must be a positive number, got: {timeout}")
- self._driver.set_script_timeout(timeout.total_seconds())
- return self._driver.execute_script(script, *arguments)
+ self._private_driver.set_script_timeout(timeout.total_seconds())
+ return self._private_driver.execute_script(script, *arguments)
except selenium.common.exceptions.WebDriverException as e:
# pylint: disable=raise-missing-from
raise ValueError(f"Could not execute JS: {e.msg}")
def close_all_tabs(self) -> None:
try:
- all_handles = self._driver.window_handles
+ all_handles = self._private_driver.window_handles
for handle in all_handles:
- self._driver.switch_to.window(handle)
- self._driver.close()
+ self._private_driver.switch_to.window(handle)
+ self._private_driver.close()
except (selenium.common.exceptions.InvalidSessionIdException,
urllib3.exceptions.MaxRetryError) as e:
logging.debug("%s: Got errors while closing all tabs: {%s}", self, e)
@@ -255,7 +253,7 @@
try:
try:
# Close the current window.
- self._driver.close()
+ self._private_driver.close()
time.sleep(0.1)
except selenium.common.exceptions.NoSuchWindowException:
# No window is good.
@@ -264,12 +262,12 @@
# Closing the last tab will close the session as well.
return
try:
- self._driver.quit()
+ self._private_driver.quit()
except selenium.common.exceptions.InvalidSessionIdException:
return
# Sometimes a second quit is needed, ignore any warnings there
try:
- self._driver.quit()
+ self._private_driver.quit()
except Exception as e: # pylint: disable=broad-except
logging.debug("Driver raised exception on quit: %s\n%s", e,
traceback.format_exc())
@@ -285,7 +283,7 @@
def __init__(self, label: str, driver: webdriver.Remote) -> None:
super().__init__(label=label, path=None)
- self._driver = driver
+ self._private_driver = driver
self.version: str = driver.capabilities["browserVersion"]
self.major_version: int = int(self.version.split(".")[0])
@@ -317,12 +315,13 @@
# Driver has already been started. We just need to mark it as running.
self._is_running = True
if self.viewport.is_fullscreen:
- self._driver.fullscreen_window()
+ self._private_driver.fullscreen_window()
elif self.viewport.is_maximized:
- self._driver.maximize_window()
+ self._private_driver.maximize_window()
else:
- self._driver.set_window_position(self.viewport.x, self.viewport.y)
- self._driver.set_window_size(self.viewport.width, self.viewport.height)
+ self._private_driver.set_window_position(self.viewport.x, self.viewport.y)
+ self._private_driver.set_window_size(self.viewport.width,
+ self.viewport.height)
def quit(self) -> None:
# External code that started the driver is responsible for shutting it down.
diff --git a/crossbench/runner/actions.py b/crossbench/runner/actions.py
index dc02a56..c000c40 100644
--- a/crossbench/runner/actions.py
+++ b/crossbench/runner/actions.py
@@ -83,6 +83,12 @@
def _assert_is_active(self) -> None:
assert self._is_active, "Actions have to be used in a with scope"
+ def current_window_id(self) -> str:
+ return self._browser.current_window_id()
+
+ def switch_window(self, window_id: str) -> None:
+ self._browser.switch_window(window_id)
+
def js(self,
js_code: str,
timeout: Union[int, float, dt.timedelta] = 10,
