fixtures: remove dirty optimization for request.getfixturevalue()#14290
Open
fixtures: remove dirty optimization for request.getfixturevalue()#14290
request.getfixturevalue()#14290Conversation
Currently for each `Function` we copy the `FunctionDefinition`'s `_arg2fixturedefs`. This is done due to an optimization for a dynamic `request.getfixturevalue()` when the fixture name wasn't statically request. In this case, we save the dynamically-found `FixtureDef` in `_arg2fixturedef`, such that if it is requested again in the same item, it is returned immediately instead of doing a `_matchfactories` check again. I've always disliked this copy and mutation. The `_arg2fixturedefs` shenanigans performed during collection are hard enough to follow, and this only adds to the complexity, due to the mutability and having multiple different `_arg2fixturedefs` with different contents. So summing up: Pros: faster repeated `request.getfixturevalue()` in same test Cons: complexity (reasoning about mutability), extra copy Since `request.getfixturevalue()` is mostly a last-resort thing, so shouldn't be too common, and *repeated* calls to it in the same test should be even less common, and if so `_matchfactories` shouldn't be *that* slow, let's remove this optimization. If this causes a noticeable slowdown for someone, we can consider bringing it back, though perhaps in a cleaner way.
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
1 participant
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.
Currently for each
Functionwe copy theFunctionDefinition's_arg2fixturedefs. This is done due to an optimization for a dynamicrequest.getfixturevalue()when the fixture name wasn't statically request. In this case, we save the dynamically-foundFixtureDefin_arg2fixturedef, such that if it is requested again in the same item, it is returned immediately instead of doing a_matchfactoriescheck again.I've always disliked this copy and mutation. The
_arg2fixturedefsshenanigans performed during collection are hard enough to follow, and this only adds to the complexity, due to the mutability and having multiple different_arg2fixturedefswith different contents.So summing up:
Pros: faster repeated
request.getfixturevalue()in same testCons: complexity (reasoning about mutability), extra copy
Since
request.getfixturevalue()is mostly a last-resort thing, so shouldn't be too common, and repeated calls to it in the same test should be even less common, and if so_matchfactoriesshouldn't be that slow, let's remove this optimization. If this causes a noticeable slowdown for someone, we can consider bringing it back, though perhaps in a cleaner way.