Skip to content

fixtures: fix quadratic collection from visibility-ordered fixture lookup - #14950

Draft
RonnyPfannschmidt wants to merge 6 commits into
pytest-dev:mainfrom
RonnyPfannschmidt:exp-14942-index-fixturedefs-by-node
Draft

fixtures: fix quadratic collection from visibility-ordered fixture lookup#14950
RonnyPfannschmidt wants to merge 6 commits into
pytest-dev:mainfrom
RonnyPfannschmidt:exp-14942-index-fixturedefs-by-node

Conversation

@RonnyPfannschmidt

@RonnyPfannschmidt RonnyPfannschmidt commented Aug 28, 2026

Copy link
Copy Markdown
Member

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 walking iter_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:

  1. fixtures: avoid hashing Nodes when matching fixturedefs — 9.1.0 also made _matchfactories() match nodes rather than baseid strings, so its inner loop hashes a Node per fixturedef per lookup and Node.__hash__ is a Python-level function (0.363s → 1.080s on 8M iterations). Precompute a _match_keyid(node), or the baseid for legacy fixturedefs — so it is one set lookup again. Small, independent regression fix.

  2. 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, so getfixturedefs() 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 makes is_visibility_more_specific() unnecessary. Includes the mutation tracking (below).

  3. fixtures: cover the whole _arg2fixturedefs mutation surface — drives every mutator and asserts the lookup result after each.

  4. 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 _FixtureDefsList method fails this test; the deterministic ones catch only their own shape.

  5. fixtures: cover legacy baseid visibility, and drop dead _matchfactories — see below.

  6. 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.

tree shape 9.0.3 main #14949 this PR
all classes at one depth 1.53s 7.41s 1.83s 1.36s
depths 1–4, interleaved 1.70s 8.72s 6.31s 1.57s

Scaling on the flat shape, where the pre-9.1 quadratic shows up:

N sibling classes 9.0.3 this PR
4000 1.52s 1.36s
8000 3.72s 2.63s
16000 10.95s 5.25s

(--collect-only, CPython 3.10, wall clock.) At N=16000 the profile has no quadratic term left; the top entries are parsefactories() 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 _arg2fixturedefs directly, bypassing _register_fixture().

A survey of the 2076 plugins in plugin_list.rst plus GitHub code search found seven doing this, between them using every mutation shape there is:

plugin shapes
pytest-bdd [k] = list, del [k], setdefault(k,[]).append(fd), [k].remove(fd)
pytest-bdd-ng setdefault(k,[]).insert(0, fd), [k].remove(fd)
pytest-psqlgraph setdefault(k,[]).insert(0, fd), [k].remove(fd)
pytest-codspeed ["benchmark"] = ...
pytest-keyring [k] = [...]
pytest-fixture-forms [k] = [fixture_def]
pykiso [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 _arg2fixturedefs and 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. 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 to begin with. Giving Node an 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.node holds the node for as long as the fixturedef that carries the key.
  • Aliasing — one silent behaviour change. Coercing assigned values to _FixtureDefsList copies, 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. _arg2fixturedefs is 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 once getfixturedefs() 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 in deprecated_test.py keeps testing what it was actually about, baseid string matching, through the real lookup path.

RonnyPfannschmidt and others added 4 commits August 28, 2026 20:18
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
RonnyPfannschmidt force-pushed the exp-14942-index-fixturedefs-by-node branch from cb73b6b to d923768 Compare August 28, 2026 18:28
@RonnyPfannschmidt RonnyPfannschmidt changed the title WIP: fixtures: index fixturedefs by visibility instead of scanning fixtures: fix quadratic collection from visibility-ordered fixture lookup Aug 28, 2026
RonnyPfannschmidt and others added 2 commits August 28, 2026 20:48
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test collection is 10x slower on 9.1.1 compared to 9.0.3

1 participant