From aa0e2d654fb0c8ac18747fe1cdf54d8f29bcd24a Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sat, 24 Oct 2020 02:09:28 +0300 Subject: [PATCH 1/3] fixtures: use a faster replacement for ischildnode ischildnode can be quite hot in some cases involving many fixtures. However it is always used in a way that the nodeid is constant and the baseid is iterated. So we can save work by pre-computing the parents of the nodeid and use a simple containment test. The `_getautousenames` function has the same stuff open-coded, so change it to use the new function as well. --- src/_pytest/fixtures.py | 11 +++---- src/_pytest/nodes.py | 68 ++++++++++++++++++----------------------- testing/test_nodes.py | 29 ++++++++++-------- 3 files changed, 51 insertions(+), 57 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 94171b5f6..6bd9e4cd6 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1478,14 +1478,10 @@ class FixtureManager: def _getautousenames(self, nodeid: str) -> List[str]: """Return a list of fixture names to be used.""" + parentnodeids = set(nodes.iterparentnodeids(nodeid)) autousenames: List[str] = [] for baseid, basenames in self._nodeid_and_autousenames: - if nodeid.startswith(baseid): - if baseid: - i = len(baseid) - nextchar = nodeid[i : i + 1] - if nextchar and nextchar not in ":/": - continue + if baseid in parentnodeids: autousenames.extend(basenames) return autousenames @@ -1668,6 +1664,7 @@ class FixtureManager: def _matchfactories( self, fixturedefs: Iterable[FixtureDef[Any]], nodeid: str ) -> Iterator[FixtureDef[Any]]: + parentnodeids = set(nodes.iterparentnodeids(nodeid)) for fixturedef in fixturedefs: - if nodes.ischildnode(fixturedef.baseid, nodeid): + if fixturedef.baseid in parentnodeids: yield fixturedef diff --git a/src/_pytest/nodes.py b/src/_pytest/nodes.py index 6ab08953a..dd58d5df9 100644 --- a/src/_pytest/nodes.py +++ b/src/_pytest/nodes.py @@ -1,6 +1,5 @@ import os import warnings -from functools import lru_cache from pathlib import Path from typing import Callable from typing import Iterable @@ -44,46 +43,39 @@ SEP = "/" tracebackcutdir = py.path.local(_pytest.__file__).dirpath() -@lru_cache(maxsize=None) -def _splitnode(nodeid: str) -> Tuple[str, ...]: - """Split a nodeid into constituent 'parts'. +def iterparentnodeids(nodeid: str) -> Iterator[str]: + """Return the parent node IDs of a given node ID, inclusive. - Node IDs are strings, and can be things like: - '' - 'testing/code' - 'testing/code/test_excinfo.py' - 'testing/code/test_excinfo.py::TestFormattedExcinfo' + For the node ID - Return values are lists e.g. - [] - ['testing', 'code'] - ['testing', 'code', 'test_excinfo.py'] - ['testing', 'code', 'test_excinfo.py', 'TestFormattedExcinfo'] + "testing/code/test_excinfo.py::TestFormattedExcinfo::test_repr_source" + + the result would be + + "" + "testing" + "testing/code" + "testing/code/test_excinfo.py" + "testing/code/test_excinfo.py::TestFormattedExcinfo" + "testing/code/test_excinfo.py::TestFormattedExcinfo::test_repr_source" + + Note that :: parts are only considered at the last / component. """ - if nodeid == "": - # If there is no root node at all, return an empty list so the caller's - # logic can remain sane. - return () - parts = nodeid.split(SEP) - # Replace single last element 'test_foo.py::Bar' with multiple elements - # 'test_foo.py', 'Bar'. - parts[-1:] = parts[-1].split("::") - # Convert parts into a tuple to avoid possible errors with caching of a - # mutable type. - return tuple(parts) - - -def ischildnode(baseid: str, nodeid: str) -> bool: - """Return True if the nodeid is a child node of the baseid. - - E.g. 'foo/bar::Baz' is a child of 'foo', 'foo/bar' and 'foo/bar::Baz', - but not of 'foo/blorp'. - """ - base_parts = _splitnode(baseid) - node_parts = _splitnode(nodeid) - if len(node_parts) < len(base_parts): - return False - return node_parts[: len(base_parts)] == base_parts + pos = 0 + sep = SEP + yield "" + while True: + at = nodeid.find(sep, pos) + if at == -1 and sep == SEP: + sep = "::" + elif at == -1: + if nodeid: + yield nodeid + break + else: + if at: + yield nodeid[:at] + pos = at + len(sep) _NodeType = TypeVar("_NodeType", bound="Node") diff --git a/testing/test_nodes.py b/testing/test_nodes.py index f9026ec61..b72a94ebe 100644 --- a/testing/test_nodes.py +++ b/testing/test_nodes.py @@ -1,3 +1,5 @@ +from typing import List + import py import pytest @@ -6,21 +8,24 @@ from _pytest.pytester import Testdir @pytest.mark.parametrize( - "baseid, nodeid, expected", + ("nodeid", "expected"), ( - ("", "", True), - ("", "foo", True), - ("", "foo/bar", True), - ("", "foo/bar::TestBaz", True), - ("foo", "food", False), - ("foo/bar::TestBaz", "foo/bar", False), - ("foo/bar::TestBaz", "foo/bar::TestBop", False), - ("foo/bar", "foo/bar::TestBop", True), + ("", [""]), + ("a", ["", "a"]), + ("aa/b", ["", "aa", "aa/b"]), + ("a/b/c", ["", "a", "a/b", "a/b/c"]), + ("a/bbb/c::D", ["", "a", "a/bbb", "a/bbb/c", "a/bbb/c::D"]), + ("a/b/c::D::eee", ["", "a", "a/b", "a/b/c", "a/b/c::D", "a/b/c::D::eee"]), + # :: considered only at the last component. + ("::xx", ["", "::xx"]), + ("a/b/c::D/d::e", ["", "a", "a/b", "a/b/c::D", "a/b/c::D/d", "a/b/c::D/d::e"]), + # : alone is not a separator. + ("a/b::D:e:f::g", ["", "a", "a/b", "a/b::D:e:f", "a/b::D:e:f::g"]), ), ) -def test_ischildnode(baseid: str, nodeid: str, expected: bool) -> None: - result = nodes.ischildnode(baseid, nodeid) - assert result is expected +def test_iterparentnodeids(nodeid: str, expected: List[str]) -> None: + result = list(nodes.iterparentnodeids(nodeid)) + assert result == expected def test_node_from_parent_disallowed_arguments() -> None: From d6becfa177a8105696fb6fd16777312a5e9c601e Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sat, 24 Oct 2020 02:17:33 +0300 Subject: [PATCH 2/3] fixtures: change _getautousenames to an iterator This reads better. --- src/_pytest/fixtures.py | 10 ++++------ testing/python/fixtures.py | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 6bd9e4cd6..644db3603 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1476,14 +1476,12 @@ class FixtureManager: self.parsefactories(plugin, nodeid) - def _getautousenames(self, nodeid: str) -> List[str]: - """Return a list of fixture names to be used.""" + def _getautousenames(self, nodeid: str) -> Iterator[str]: + """Return the names of autouse fixtures applicable to nodeid.""" parentnodeids = set(nodes.iterparentnodeids(nodeid)) - autousenames: List[str] = [] for baseid, basenames in self._nodeid_and_autousenames: if baseid in parentnodeids: - autousenames.extend(basenames) - return autousenames + yield from basenames def getfixtureclosure( self, @@ -1499,7 +1497,7 @@ class FixtureManager: # (discovering matching fixtures for a given name/node is expensive). parentid = parentnode.nodeid - fixturenames_closure = self._getautousenames(parentid) + fixturenames_closure = list(self._getautousenames(parentid)) def merge(otherlist: Iterable[str]) -> None: for arg in otherlist: diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index bd82125e7..a4838ee51 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -1710,7 +1710,7 @@ class TestAutouseDiscovery: """ from _pytest.pytester import get_public_names def test_check_setup(item, fm): - autousenames = fm._getautousenames(item.nodeid) + autousenames = list(fm._getautousenames(item.nodeid)) assert len(get_public_names(autousenames)) == 2 assert "perfunction2" in autousenames assert "perfunction" in autousenames From 470ea504e2227f879103782b76810447b1923214 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sat, 24 Oct 2020 00:22:13 +0300 Subject: [PATCH 3/3] fixtures: fix quadratic behavior in the number of autouse fixtures It turns out all autouse fixtures are kept in a global list, and thinned out for a particular node using a linear scan of the entire list each time. Change the list to a dict, and only take the nodes we need. --- changelog/4824.bugfix.rst | 1 + src/_pytest/fixtures.py | 15 ++++++++------- 2 files changed, 9 insertions(+), 7 deletions(-) create mode 100644 changelog/4824.bugfix.rst diff --git a/changelog/4824.bugfix.rst b/changelog/4824.bugfix.rst new file mode 100644 index 000000000..f2e6db7ab --- /dev/null +++ b/changelog/4824.bugfix.rst @@ -0,0 +1 @@ +Fixed quadratic behavior and improved performance of collection of items using autouse fixtures and xunit fixtures. diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 644db3603..18094f21c 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1412,9 +1412,10 @@ class FixtureManager: self.config: Config = session.config self._arg2fixturedefs: Dict[str, List[FixtureDef[Any]]] = {} self._holderobjseen: Set[object] = set() - self._nodeid_and_autousenames: List[Tuple[str, List[str]]] = [ - ("", self.config.getini("usefixtures")) - ] + # A mapping from a nodeid to a list of autouse fixtures it defines. + self._nodeid_autousenames: Dict[str, List[str]] = { + "": self.config.getini("usefixtures"), + } session.config.pluginmanager.register(self, "funcmanage") def _get_direct_parametrize_args(self, node: nodes.Node) -> List[str]: @@ -1478,9 +1479,9 @@ class FixtureManager: def _getautousenames(self, nodeid: str) -> Iterator[str]: """Return the names of autouse fixtures applicable to nodeid.""" - parentnodeids = set(nodes.iterparentnodeids(nodeid)) - for baseid, basenames in self._nodeid_and_autousenames: - if baseid in parentnodeids: + for parentnodeid in nodes.iterparentnodeids(nodeid): + basenames = self._nodeid_autousenames.get(parentnodeid) + if basenames: yield from basenames def getfixtureclosure( @@ -1642,7 +1643,7 @@ class FixtureManager: autousenames.append(name) if autousenames: - self._nodeid_and_autousenames.append((nodeid or "", autousenames)) + self._nodeid_autousenames.setdefault(nodeid or "", []).extend(autousenames) def getfixturedefs( self, argname: str, nodeid: str