Skip to content

fixtures: fix quadratic collection slowdown from visibility-ordered override chains - #14949

Closed
RonnyPfannschmidt wants to merge 1 commit into
pytest-dev:mainfrom
RonnyPfannschmidt:fix-14942-fixture-order-perf
Closed

fixtures: fix quadratic collection slowdown from visibility-ordered override chains#14949
RonnyPfannschmidt wants to merge 1 commit into
pytest-dev:mainfrom
RonnyPfannschmidt:fix-14942-fixture-order-perf

Conversation

@RonnyPfannschmidt

Copy link
Copy Markdown
Member

Fixes #14942.

AI-assisted: the analysis, patch and benchmarks were produced by Claude Opus 5 via Claude Code, driven and reviewed by me. Attribution is in the commit trailers. Detailed profiling writeup in #14942 (comment).

The regressions

Two, both introduced in 9.1.0.

1. _register_fixture scans the whole list on every registration.

Ordering the override chain by visibility (7186cd4, #14513) inserts by scanning:

for i, existing in enumerate(faclist):
    if is_visibility_more_specific(existing, fixture_def):
        faclist.insert(i, fixture_def)
        break
else:
    faclist.append(fixture_def)

For a fixture name registered on very many nodes — a fixture inherited from a base class by thousands of test classes, which is the reporter's layout — that is O(n²) is_visibility_more_specific() calls, each walking iter_parents(). With 4000 such classes: 7,998,000 calls, all returning False, 11.5s of a 16.2s profiled run.

An existing fixturedef can only be more specific than the new one if its node is strictly deeper in the collection tree. So track the deepest registration per fixture name and skip the scan when nothing registered so far can be deeper. Fixturedefs with a legacy string baseid have no node, so a name that has seen one keeps doing the full scan.

Collection is top-down, so the scan effectively only fires for plugins that register fixtures out of tree order — which is what test_register_fixture_ordered_by_visibility exercises (node=item before node=sessionmax_depth > depth ⇒ scan runs). Ordering behaviour is unchanged.

2. Node.__hash__ in _matchfactories.

With (1) fixed, 4000 classes is still 4.62s profiled against 9.0.3's 2.13s. _matchfactories is O(defs-with-that-name) per lookup in both versions — that quadratic is not new — but node-based matching made the inner loop much more expensive:

parent_nodes = set(node.iter_parents())
...
if fixturedef.node in parent_nodes:

Node.__hash__ is a Python-level function (hash(self._nodeid)), so each of the 8M iterations now costs a Python call, where 9.0.3 did one attribute access and one str set lookup. 0.363s → 1.080s.

Precompute a _match_key on FixtureDef: id(node) for node-based defs, the baseid string for legacy ones. Nodes compare by identity and the fixturedef holds its node alive, so the id cannot be reused; int and str never compare equal, so a single set holds both kinds and the loop is one set lookup again.

Numbers

--collect-only over N sibling classes each defining the same fixture name, CPython 3.10, wall clock, no profiler (reproducer):

N classes 9.0.3 main this PR
1000 0.38s 0.98s 0.56s
2000 0.67s 2.22s 0.94s
4000 1.53s 7.41s 1.83s

Deliberately not in this PR

_matchfactories is still O(defs-with-that-name) per lookup, in 9.0.3 and after this PR alike. Removing that means indexing _arg2fixturedefs[name] by node and iterating the short parent chain instead of the long fixturedef list — which would also subsume the _register_fixture ordering, since the fixturedefs visible to a node form a chain and are therefore totally ordered. That is a behaviour-shaped change rather than a pure optimisation, so it gets its own draft PR; this one stays a regression fix.

pytest 9.1.0 started ordering the fixturedef list of each fixture name by
visibility (7186cd4, pytest-dev#14513). The insertion scans the whole list on every
registration, so a suite that defines the same fixture name on very many nodes
-- e.g. a fixture inherited from a base class by thousands of test classes --
pays O(n^2) `is_visibility_more_specific()` calls, each walking `iter_parents()`.

An existing fixturedef can only be more specific than the new one if it is
defined strictly deeper in the collection tree, so track the deepest
registration per fixture name and skip the scan when no existing fixturedef can
possibly be deeper. Fixturedefs with a legacy string baseid have no node, so
those fall back to the full scan.

While profiling this, node-based matching in `_matchfactories()` turned out to
be a second, smaller regression from the same release: it hashes a `Node` per
fixturedef per lookup, and `Node.__hash__` is a Python-level function. Give
`FixtureDef` a precomputed `_match_key` -- `id(node)`, or the baseid for legacy
fixturedefs -- so the loop is a single set lookup, as it was before.

Collection of 4000 sibling classes each defining the same fixture name:
9.0.3 1.53s, 9.1.x/main 7.41s, with this change 1.83s.

Fix pytest-dev#14942

Co-Authored-By: Claude Opus 5 (1M context) <ai@anthropic.com>
Co-Authored-By: Claude Code <ai@anthropic.com>
@RonnyPfannschmidt

Copy link
Copy Markdown
Member Author

Closing in favour of #14950, which now contains the salvageable part of this branch as its first commit.

AI-assisted, as before: analysis and patches by Claude Opus 5 via Claude Code, driven and reviewed by me.

This PR did not fix the reported problem. @mikicz tried it against their suite and collection stayed at ~5 minutes; the analysis is here. In short: the registration-scan guard skips the scan only while fixturedefs arrive in non-decreasing collection-tree depth. My reproducer was 4000 sibling classes in a single module — all at the same depth — so the guard skipped ~100% of scans and the fix looked complete. Real suites have test classes at many directory depths and pytest collects depth-first, so on a tree with depths 1–4 the guard skips 27% of registrations and buys almost nothing (8.72s vs 8.7s unpatched, against 9.0.3's 1.70s).

The _matchfactories half of this PR was a real fix and survives as the first commit of #14950. The depth guard is gone.

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