Reduce cloning in TestVariantIterator#683
Merged
MatthewMckee4 merged 1 commit intomainfrom Apr 15, 2026
Merged
Conversation
Rework `TestVariantIterator` so producing a variant is cheap. The iterator now borrows `&DiscoveredTestFunction` from the surrounding module instead of rebuilding it into an `Rc`, and shares fixture lists across variants via `Rc<[Rc<NormalizedFixture>]>` so each variant costs a handful of refcount bumps rather than a full `Vec` clone per fixture set. `param_args` is consumed as an `IntoIter`, moving each `ParametrizationArgs.values` (and `.tags`) into the emitted variant; this also lets `Arc::try_unwrap` in `setup_test_fixtures` succeed, skipping a Python refcount bump per parametrize argument. Implements `Iterator` on `TestVariantIterator` so the call site in `package_runner` can use a plain `for variant in ...` loop. Also collapses a couple of `match` arms in `package_runner` into `map_err(...)?` and skips building the kwargs `PyDict` in `NormalizedFixture::call` when there are no fixture arguments.
Merging this PR will not alter performance
|
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.
Summary
TestVariantIteratorsits on the hot path for every test invocation — once per parametrize variant, per fixture set, per test. It was doing more copying than it needed to.The big change is ownership. Before, constructing the iterator rebuilt the whole
DiscoveredTestFunctioninto anRcjust so each emitted variant could hold a shared pointer to it:But the caller already owns the
DiscoveredTestFunctioninsidemodule.test_functions(), and that outlives the iterator. So the iterator now borrows&'a DiscoveredTestFunctiondirectly, andTestVariant<'a>carries that same borrow. The rebuild and its three clones (name, tags, py_function refcount bump) are gone.The fixture lists are the other big win. A test with N parametrize variants and M fixtures used to allocate N fresh
Vec<Rc<NormalizedFixture>>and bump N × M refcounts pernext()call. Wrapping them inRc<[Rc<NormalizedFixture>]>reduces each variant to a single refcount bump on the outerRc, regardless of how many fixtures are in the list.param_argsis now consumed as astd::vec::IntoIter<ParametrizationArgs>. Each variant moves the owningvalues: HashMap<String, Arc<Py<PyAny>>>andtags: Tagsout by value instead of cloning them. This has a follow-on effect insetup_test_fixtures:Arc::try_unwrap(value)used to always fall back toclone_ref(py)because the clone kept the refcount at ≥2, but with the move it hits the fast path and skips a Python refcount bump per parametrize argument.With these changes,
next()becomes the natural shape of an iterator, so the type now implementsIterator(plussize_hint/ExactSizeIteratorfrom the underlyingIntoIter), and the call site inpackage_runneruses a plainfor variant in ...loop instead ofwhile let Some(variant) = iterator.next_with_py(py).Two small drive-by cleanups landed alongside the main change: a couple of
matcharms inpackage_runnerthat reshapedErrintoFixtureCallErrorcollapsed intomap_err(|err| FixtureCallError { ... })?, andNormalizedFixture::callnow only builds the kwargsPyDictwhen there are actually fixture arguments to pass.Test Plan
just test— full suite passes (parametrize, fixtures, autouse, use_fixtures, snapshot coverage all exercise this code path)cargo clippy -p karva_test_semantic --lib -- -D warnings— cleanuvx prek run -a