fixtures: use a better data structure for storing FixtureDefs - #14984
fixtures: use a better data structure for storing FixtureDefs#14984bluetech wants to merge 5 commits into
Conversation
Let's use consistent terminology.
This is an internal function, so it's not a official guarantee, but let's explicitly document the behavior internally at least.
psf-chronographer
Bot
added
the
bot:chronographer:provided
…ide the class It's easier to handle this way.
690d7d4 to
d938a56
Compare
| # Legacy fallback, for plugins still using the deprecated nodeid-based | ||
| # API without a node reference. | ||
| # Part of FIXTURE_NODEID_DEPRECATED deprecation. | ||
| self._arg2nodeid2fixturedefs: Final[ |
There was a problem hiding this comment.
Instead of a separate datastructure, is it possible for us to get the nodeid from the session collection tree? Or perhaps the node might not exist at this point yet?
There was a problem hiding this comment.
If you mean that the single data structure will work with nodeids, it's possible and I think it will work, but then we're back to working with nodeids and I want to avoid that.
If you mean going the other way, i.e. getting a Node given a nodeid, we don't have a way to do it other than searching through the collection tree, which will be slow.
I went with a similar solution to what we did for autouse names (_nodeid_autousenames). In pytest 10.1 we just delete all of that...
There was a problem hiding this comment.
Yeah I meant the 2nd one (only using Node) -- but I agree with the solution then, as we will eventually just delete the 2nd datastructure once we drop that support. 👍
Previously, FixtureManager stored the registered FixtureDefs in
`_arg2fixturedefs` which is
<fixutre name> -> [FixtureDef]
where the FixtureDefs are ordered by visibility.
There are two inefficiencies with this:
1. When registering a fixture, we need to find the appropriate index to
insert in the list. This is done with slow quadratic
`is_visibility_more_specific` checks.
Before 7186cd4, FixtureDefs were
always appended, relying on the collection order, so there was no
quadratic issue. But then we added `pytest.register_fixture` which is
not guaranteed to be called in collection order.
This is pytest-dev#14942, introduced in v9.1.0.
2. When looking up FixtureDefs for a node, the entire list needed to be
filtered for visibility to the node (`_matchfactories`). With many
fixtures registered with the same name (even if completely
unrelated), this can be slow.
This is an old issue.
Change the way we store the FixtureDefs to `_arg2node2fixturedefs`,
which is
<fixture name> -> (Node -> [FixtureDef])
i.e. instead of storing the FixtureDefs for a name in a single big list,
store them by the Node under which they are registered.
This fixes (1) since now just need to append to
`arg2fixture2nodes[name][node]`.
Fixes (2) since no longer need to filter a big list (scaling with number
of fixtures registered with same name). Instead need to look up the
FixtureDefs registered for the node and its ancestors (scales with
height of the collection tree, which should be OK).
Fix pytest-dev#14942
d938a56 to
aeb5b3e
Compare
|
@nicoddemus I fixed the changelog, and also added a couple of tests for coverage of the nodeid paths. The remaining coverage miss is preexisting, which look to me like a leftover from a reverted "invocation-scoped fixtures" feature which was before my time (code added in 44ecf2a). |
|
Cool, LGTM! |
|
@nicoddemus missclick? |
|
Ouch yes! Probably hit "Enter" at the wrong moment, apologies. |
3 participants
This PR fixes #14942.
The commits
The 1st and 2nd commits are related internal documentation fixes.
The 3rd commit is a little refactor to encapsulate access to
FixtureManager._arg2fixturedefto make the next commit clearer.The 4th commit is main change, I reproduce the commit message below.
The 5th commit adds backward compat for string nodeids in
register_fixture. I split it from the previous commit to make it easier to review the main change without the ugly compat. I will squash it before merging.Description
Previously, FixtureManager stored the registered FixtureDefs in
_arg2fixturedefswhich iswhere the FixtureDefs are ordered by visibility.
There are two inefficiencies with this:
When registering a fixture, we need to find the appropriate index to insert in the list. This is done with slow quadratic
is_visibility_more_specificchecks.Before 7186cd4, FixtureDefs were always appended, relying on the collection order, so there was no quadratic issue. But then we added
pytest.register_fixturewhich is not guaranteed to be called in collection order.This is Test collection is 10x slower on 9.1.1 compared to 9.0.3 #14942, introduced in v9.1.0.
When looking up FixtureDefs for a node, the entire list needed to be filtered for visibility to the node (
_matchfactories). With many fixtures registered with the same name (even if completely unrelated), this can be slow.This is an old issue.
Change the way we store the FixtureDefs to
_arg2node2fixturedefs, which isi.e. instead of storing the FixtureDefs for a name in a single big list, store them by the Node under which they are registered.
This fixes (1) since now just need to append to
arg2fixture2nodes[name][node].Fixes (2) since no longer need to filter a big list (scaling with number of fixtures registered with same name). Instead need to look up the FixtureDefs registered for the node and its ancestors (scales with height of the collection tree, which should be OK).
Performance
Performance numbers with reproducer from #14942 (comment) (
NCLASSES=10000 time pytest reproducer.py):It's still kinda slow, but a profile shows that's for other preexisting reasons. The improvement over main is due to fixing problem (1), and over before the regression due to fixing problem (2).
Backward compat
This breaks plugins which directly access
FixtureManager._arg2fixturedefs(double private 😀). From my local corpus (678 plugins), I see it done in these plugins:I think this is acceptable.