fixtures: fix quadratic collection from visibility-ordered fixture lookup - #14950
Draft
RonnyPfannschmidt wants to merge 6 commits into
Draft
fixtures: fix quadratic collection from visibility-ordered fixture lookup#14950RonnyPfannschmidt wants to merge 6 commits into
RonnyPfannschmidt wants to merge 6 commits into
Conversation
pytest 9.1.0 moved `_matchfactories()` from matching a fixturedef's baseid string against the requesting node's parent nodeids to matching its node against the parent nodes themselves. `Node.__hash__` is a Python-level function (`hash(self._nodeid)`), so the inner loop went from one attribute access plus a `str` set lookup to a Python call per fixturedef -- and it runs once per fixturedef per lookup. On a suite that defines one fixture name on 4000 nodes that is 8M iterations: 0.363s before, 1.080s after. Precompute a `_match_key` on `FixtureDef`: `id()` of the node for node-based fixturedefs, the baseid string for legacy ones. Nodes compare by identity and the fixturedef holds its node alive, so the id cannot be reused while it is in use as a key; `int` and `str` never compare equal, so a single set holds both kinds and the loop is one set lookup again. Ref pytest-dev#14942 Co-Authored-By: Claude Opus 5 (1M context) <ai@anthropic.com> Co-Authored-By: Claude Code <ai@anthropic.com>
`getfixturedefs()` filtered the full list of fixturedefs registered under a
name down to the ones visible to the requesting node. That list is as long as
the number of definitions of that name in the whole suite, so a fixture
inherited from a base class by thousands of test classes made collection
quadratic.
The fixturedefs visible to a node are exactly those defined on the node or one
of its ancestors, and the ancestor chain is short. So bucket the fixturedefs by
their visibility key and walk the parent chain instead of the definition list.
Since the visible set is a chain, it is totally ordered by visibility, so
`getfixturedefs()` can order the override chain itself: most general first,
ties broken by registration order. That subsumes the partial-order insertion
`_register_fixture()` was maintaining, which can now just append, and makes
`is_visibility_more_specific()` unnecessary.
The index cannot simply mirror `_register_fixture()`, because that is not the
only way fixturedefs enter the manager: `_arg2fixturedefs` is de-facto public
API that plugins mutate directly to inject fixtures. A survey of the plugin
list found seven doing so -- pytest-bdd, pytest-bdd-ng, pytest-psqlgraph,
pytest-codspeed, pytest-keyring, pytest-fixture-forms and pykiso -- between
them using whole-key assignment, `del`, `setdefault().append()`,
`insert(0, ...)` and `remove()`.
So `_arg2fixturedefs` and its lists notice. An append extends the index in
place, since it only adds a fixturedef with the next ordinal -- that is the hot
path, taken once per fixture in the suite. Every other mutation drops the
name's index, which `_index_for()` rebuilds on the next lookup.
Validating by list length instead would have been cheaper and wrong: pytest-bdd
appends a fixturedef and removes it again in a finalizer, leaving the same list
object at the same length with different contents.
Collection of N sibling classes each defining the same fixture name, and of
4000 such classes spread over a tree of varying depth:
9.0.3 9.1.x this
flat, N=4000 1.52s 7.41s 1.36s
flat, N=16000 10.95s 10.83s 5.25s
varying depth 1.70s 8.72s 1.57s
Fix pytest-dev#14942
Co-Authored-By: Claude Opus 5 (1M context) <ai@anthropic.com>
Co-Authored-By: Claude Code <ai@anthropic.com>
The previous commit tracks every way `_arg2fixturedefs` and its lists can be mutated, but only four of those ways are used by the plugins that prompted it, so the rest went untested -- and an untested invalidation path is exactly the kind that rots into a stale index. Drive each of them through the live fixture manager and assert the lookup result after every step. `list.clear()` is exercised on a throwaway mapping since on the live one it would drop every fixture in the session. Together with the plugin-shape test this covers every line of `_FixtureDefsList`, `_Arg2FixtureDefs`, `_index_for()` and `getfixturedefs()`. Co-Authored-By: Claude Opus 5 (1M context) <ai@anthropic.com> Co-Authored-By: Claude Code <ai@anthropic.com>
The two deterministic tests cover the mutation shapes one at a time. What they do not cover is sequences: an append onto a list whose index was just dropped, an assignment followed by an append, a remove between two lookups. That is where a stale index actually comes from. Drive those sequences with a hypothesis state machine, and after every step assert that `getfixturedefs()` agrees with the same question answered the naive way -- scan the authoritative `_arg2fixturedefs` list, keep what is defined on the requesting node or an ancestor, order it most general first. The nodes are stand-ins rather than collected ones: visibility only depends on identity, nodeid and the parent chain, so a real tree would cost a collection per example without testing anything more. Dropping the `_invalidate()` from any single `_FixtureDefsList` method fails this test; the deterministic ones catch only their own shape. Co-Authored-By: Claude Opus 5 (1M context) <ai@anthropic.com> Co-Authored-By: Claude Code <ai@anthropic.com>
RonnyPfannschmidt
force-pushed
the
exp-14942-index-fixturedefs-by-node
branch
from
August 28, 2026 18:28
cb73b6b to
d923768
Compare
Two loose ends the coverage of the previous commits exposed. The property test only built node-based fixturedefs, so the reference lookup's legacy branch -- rank by baseid string rather than by node identity -- was never taken. Add a rule that registers fixturedefs with a string baseid and no node, as plugins which have not moved off the deprecated API still produce. That covers the deprecated path on both sides of the comparison. `_matchfactories()` has had no caller since `getfixturedefs()` started answering from the index. It is not part of any public API, nothing in the tree uses it, and a scan of the plugin list found no plugin calling it either -- the only hits were vendored copies of pytest itself. Remove it rather than leave a second, now-untested implementation of visibility matching for the two to drift apart. Its test in deprecated_test.py keeps testing what it was really about, which is baseid string matching, through the real lookup path. Co-Authored-By: Claude Opus 5 (1M context) <ai@anthropic.com> Co-Authored-By: Claude Code <ai@anthropic.com>
Two things a reader of the index code should not have to reconstruct. Coercing assigned values to `_FixtureDefsList` copies, so assigning one list under two fixture names now yields two independent lists, and a list held from before the assignment no longer tracks the stored one. `_arg2fixturedefs` is internal and some churn there is expected, but this particular change fails silently rather than loudly, so it is spelled out in the class docstring, in the changelog and pinned by a test. Assigning back a list read out of the mapping under the same name stays a no-op, so the usual plugin patterns are unaffected. And `id()` is a workaround, not the natural key. The natural key is the node itself, since nodes compare by identity -- but `Node.__hash__` is a Python-level function hashing `nodeid`, so keying on nodes costs a Python call per fixturedef per lookup, which is what made this path a bottleneck. Say so, along with what would let it go away: an identity hash on `Node`, which its identity `__eq__` implies anyway. Co-Authored-By: Claude Opus 5 (1M context) <ai@anthropic.com> Co-Authored-By: Claude Code <ai@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14942. Supersedes #14949, which did not fix the reported case and is now closed; its one good commit is the first commit here.
AI-assisted: the analysis, patches and benchmarks were produced by Claude Opus 5 via Claude Code, driven and reviewed by me. Attribution is in the commit trailers.
The problem
pytest 9.1.0 started ordering the fixturedef list of each fixture name by visibility (7186cd4, #14513), inserting by scanning the list on every registration. A suite that defines the same fixture name on very many nodes — a fixture inherited from a base class by thousands of test classes — pays O(n²)
is_visibility_more_specific()calls, each walkingiter_parents(). The reporter went from 30s to 5 minutes on--collect-only.Underneath that sits an older quadratic that 9.0.3 had too:
getfixturedefs()filters the whole list of definitions of that name down to the ones visible to the requesting node, once per lookup.The commits
Six, each standalone:
fixtures: avoid hashing Nodes when matching fixturedefs— 9.1.0 also made_matchfactories()match nodes rather than baseid strings, so its inner loop hashes aNodeper fixturedef per lookup andNode.__hash__is a Python-level function (0.363s → 1.080s on 8M iterations). Precompute a_match_key—id(node), or the baseid for legacy fixturedefs — so it is one set lookup again. Small, independent regression fix.fixtures: index fixturedefs by visibility instead of scanning— the actual fix. The fixturedefs visible to a node are exactly those defined on it or an ancestor, and that chain is short, so bucket them by visibility key and walk the parent chain instead of the definition list. Since the visible set is a chain it is totally ordered, sogetfixturedefs()can produce the override chain order itself — which subsumes the partial-order insertion from Fixture override order should be determined by visibility? #14513, lets_register_fixture()just append, and makesis_visibility_more_specific()unnecessary. Includes the mutation tracking (below).fixtures: cover the whole _arg2fixturedefs mutation surface— drives every mutator and asserts the lookup result after each.fixtures: property-test the visibility index against a naive lookup— a hypothesis state machine comparing the index against the same question answered by scanning. Dropping the_invalidate()from any single_FixtureDefsListmethod fails this test; the deterministic ones catch only their own shape.fixtures: cover legacy baseid visibility, and drop dead _matchfactories— see below.fixtures: document the aliasing change and the id() workaround— docstrings, changelog and a test pinning the one silent behaviour change.Numbers
4000 classes each inheriting the same fixture from a shared base class, varying only the shape of the directory tree. The second row is what real suites look like, and is the row #14949 failed on.
Scaling on the flat shape, where the pre-9.1 quadratic shows up:
(
--collect-only, CPython 3.10, wall clock.) At N=16000 the profile has no quadratic term left; the top entries areparsefactories()scanning class dicts.Why the mutation tracking
The first version of this branch broke pytest-bdd. It moved the source of truth for
getfixturedefs()into the index, and pytest-bdd injects step fixtures by writing into_arg2fixturedefsdirectly, bypassing_register_fixture().A survey of the 2076 plugins in
plugin_list.rstplus GitHub code search found seven doing this, between them using every mutation shape there is:[k] = list,del [k],setdefault(k,[]).append(fd),[k].remove(fd)setdefault(k,[]).insert(0, fd),[k].remove(fd)setdefault(k,[]).insert(0, fd),[k].remove(fd)["benchmark"] = ...[k] = [...][k] = [fixture_def][k] = [...](Read-only use is far wider — pytest-deadfixtures, pytest-unused-fixtures, pytest-fixture-tools, pytest-pyodide, ApeWorX/ape, DataDog integrations-core — but reads were never at risk.)
So
_arg2fixturedefsand its lists notice. An append extends the index in place — the hot path, once per fixture in the suite — and every other mutation drops the name's index, rebuilt on the next lookup. Reads (iteration, indexing,len) stay plain C list; only the mutating methods go through Python.Validating by list length instead would have been cheaper and wrong: pytest-bdd appends a fixturedef and removes it again in a finalizer, leaving the same list object at the same length with different contents.
testing/plugins_integration(which runs pytest-bdd) passes.Review notes
id()as an index key — a workaround, and documented as one. The natural key is the node itself, since nodes compare by identity. ButNode.__hash__is a Python-level function hashingnodeid, so keying on nodes costs a Python call per fixturedef per lookup — which is what made this path a bottleneck to begin with. GivingNodean identity hash (which its identity__eq__implies anyway) would let this go back to keying on the node; that felt like a bigger change than this PR should make. A reused id would mean matching the wrong node, so the key must not outlive the object:FixtureDef.nodeholds the node for as long as the fixturedef that carries the key._FixtureDefsListcopies, so assigning one list under two fixture names now gives two independent lists, and a list held from before the assignment no longer tracks the stored one._arg2fixturedefsis internal and some churn is expected, but this fails silently rather than loudly, so it is spelled out in the class docstring and the changelog and pinned by a test. Assigning back a list read out of the mapping under the same name stays a no-op, so the usual plugin patterns are unaffected, and no surveyed plugin relies on the aliasing.is_visibility_more_specific()is removed. Module-level in_pytest.fixtures, added in 9.1.0 by Fixture override order should be determined by visibility? #14513, no underscore prefix, nothing in-tree uses it any more. Removing it outright vs. keeping a shim is a maintainer call._matchfactories()is removed (last commit). Nothing in the tree called it oncegetfixturedefs()started answering from the index, and a scan of the plugin list found no plugin calling it either — the only hits were vendored copies of pytest itself. Leaving it would have meant a second, untested implementation of visibility matching, free to drift. Its test indeprecated_test.pykeeps testing what it was actually about, baseid string matching, through the real lookup path.