Allow immutable borrow to access QuantumCircuit.parameters
#12918
Conversation
`QuantumCircuit.parameters` is logically a read-only operation on `QuantumCircuit`. For efficiency in multiple calls to `assign_parameters`, we actually cache the sort order of the internal `ParameterTable` on access. This is purely a caching effect, and should not leak out to users. The previous implementation took a Rust-space mutable borrow out in order to (potentially) mutate the cache. This caused problems if multiple Python threads attempted to call `assign_parameters` simultaneously; it was possible for one thread to give up the GIL during its initial call to `CircuitData::copy` (so an immutable borrow was still live), allowing another thread to continue on to the getter `CircuitData::get_parameters`, which required a mutable borrow, which failed due to the paused thread in `copy`. This moves the cache into a `RefCell`, allowing the parameter getters to take an immutable borrow as the receiver. We now write the cache out only if we *can* take the mutable borrow out necessary. This can mean that other threads will have to repeat the work of re-sorting the parameters, because their borrows were blocking the saving of the cache, but this will not cause failures. The methods on `ParameterTable` that invalidate the cache all require a mutable borrow on the table itself. This makes it impossible for an immutable borrow to exist simultaneously on the cache, so these methods should always succeed to acquire the cache lock to invalidate it.
jakelishman
added
Changelog: None
|
One or more of the following people are relevant to this code:
|
jakelishman
added
the
stable backport potential
Pull Request Test Coverage Report for Build 10316796986Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
In several cases, the previous code was using the runtime-checked `RefCell::borrow_mut` in locations that can be statically proven to be safe to take the mutable reference. Using the correct function for this makes the logic clearer (as well as technically removing a small amount of runtime overhead).
`OnceCell` has less runtime checking than `RefCell` (only whether it is initialised or not, which is an `Option` check), and better represents the dynamic extensions to the borrow checker that we actually need for the caching in this method. All methods that can invalidate the cache all necessarily take `&mut ParameterTable` already, since they will modify Rust-space data. A `OnceCell` can be deinitialised through a mutable reference, so this is fine. The only reason a `&ParameterTable` method would need to mutate the cache is to create it, which is the allowed set of `OnceCell` operations.
There was a problem hiding this comment.
This looks good to me.
I'm much happier now with the OnceCell being used instead of RefCell, which had felt like it required a little bit too much manual coordination when managing the cache.
As you mentioned in the PR description, this won't be the last time we encounter borrowing issues between threads. In this case, our caching was the reason we needed a mutable borrow. But, in cases where we actually have these semantics (where it is the user's fault, in that they're doing something that requires multiple threads to borrow the same thing concurrently), are we planning to replicate the behavior of the old Python code (doing extra work but providing a correct result), or to produce a runtime failure?
| ) -> impl Iterator<Item = (Py<PyAny>, HashSet<ParameterUse>)> + '_ { | ||
| self.ensure_sorted(); | ||
| &mut self, | ||
| ) -> impl ExactSizeIterator<Item = (Py<PyAny>, HashSet<ParameterUse>)> { |
There was a problem hiding this comment.
Is there any reason to use an abstract return type here, or can we just return ParameterTableDrain?
In the case of Vec::drain, a public Drain struct gets returned explicitly. That seems like a useful pattern for the general case since we can then implement additional traits for the iterator if needed.
There was a problem hiding this comment.
Mostly I'd done it because I didn't want ParameterTableDrain to exist at all, so I kept the type private. The original version of this function had the iterator type unnameable, because I could borrow everything out of the struct without needing a manual implementation. The Rust stdlib exports all its iterator type objects possibly in part because anonymous impl Trait in return position only turned up in 1.26, so there was no alternative before that.
I can make it a public type if you think it's better that way, but I'm not sure I agree with the justification "can implementation additional traits [...] if needed" in this case - if we need them, then we can name the type and make it public, but at the moment, we've no need.
There was a problem hiding this comment.
I suppose it's fine as is, given that we can swap in an explicit struct if needed.
(Though I'd be curious if Rust would have used abstract return types for this kind of thing in the standard lib if they'd be around from the beginning.)
There was a problem hiding this comment.
Yeah, I don't know if they would have either. The anonymous types don't make for the prettiest reading of documentation, tbf, though equally, all the structs named Iter just mean you have to go to a mostly empty separate docs page to find which of the Iterator extension traits it implements.
Oddly, I'm unhappy with this PR because I think this actually requires more manual co-ordination when managing the cache, because now you have to take a lot of care (that the compiler won't protect you from) not to yield control to the Python interpreter during the initialiser, which makes In the meantime, I'm happy though because it uses less memory, the point-of-use semantics are clearer, and it requires less runtime checking haha. |
Can we add a code comment that warns devs of this? With |
|
Oh, I got confused with the two PRs sorry - in this particular PR, the creation of the cache object doesn't require yielding to the Python interpreter. The operations in #12942 do, though, and they include big explanatory code comments about the problem. |
* Allow immutable borrow to access `QuantumCircuit.parameters` `QuantumCircuit.parameters` is logically a read-only operation on `QuantumCircuit`. For efficiency in multiple calls to `assign_parameters`, we actually cache the sort order of the internal `ParameterTable` on access. This is purely a caching effect, and should not leak out to users. The previous implementation took a Rust-space mutable borrow out in order to (potentially) mutate the cache. This caused problems if multiple Python threads attempted to call `assign_parameters` simultaneously; it was possible for one thread to give up the GIL during its initial call to `CircuitData::copy` (so an immutable borrow was still live), allowing another thread to continue on to the getter `CircuitData::get_parameters`, which required a mutable borrow, which failed due to the paused thread in `copy`. This moves the cache into a `RefCell`, allowing the parameter getters to take an immutable borrow as the receiver. We now write the cache out only if we *can* take the mutable borrow out necessary. This can mean that other threads will have to repeat the work of re-sorting the parameters, because their borrows were blocking the saving of the cache, but this will not cause failures. The methods on `ParameterTable` that invalidate the cache all require a mutable borrow on the table itself. This makes it impossible for an immutable borrow to exist simultaneously on the cache, so these methods should always succeed to acquire the cache lock to invalidate it. * Use `RefCell::get_mut` where possible In several cases, the previous code was using the runtime-checked `RefCell::borrow_mut` in locations that can be statically proven to be safe to take the mutable reference. Using the correct function for this makes the logic clearer (as well as technically removing a small amount of runtime overhead). * Use `OnceCell` instead of `RefCell` `OnceCell` has less runtime checking than `RefCell` (only whether it is initialised or not, which is an `Option` check), and better represents the dynamic extensions to the borrow checker that we actually need for the caching in this method. All methods that can invalidate the cache all necessarily take `&mut ParameterTable` already, since they will modify Rust-space data. A `OnceCell` can be deinitialised through a mutable reference, so this is fine. The only reason a `&ParameterTable` method would need to mutate the cache is to create it, which is the allowed set of `OnceCell` operations. (cherry picked from commit 2d3db9a)
…#12958) * Allow immutable borrow to access `QuantumCircuit.parameters` `QuantumCircuit.parameters` is logically a read-only operation on `QuantumCircuit`. For efficiency in multiple calls to `assign_parameters`, we actually cache the sort order of the internal `ParameterTable` on access. This is purely a caching effect, and should not leak out to users. The previous implementation took a Rust-space mutable borrow out in order to (potentially) mutate the cache. This caused problems if multiple Python threads attempted to call `assign_parameters` simultaneously; it was possible for one thread to give up the GIL during its initial call to `CircuitData::copy` (so an immutable borrow was still live), allowing another thread to continue on to the getter `CircuitData::get_parameters`, which required a mutable borrow, which failed due to the paused thread in `copy`. This moves the cache into a `RefCell`, allowing the parameter getters to take an immutable borrow as the receiver. We now write the cache out only if we *can* take the mutable borrow out necessary. This can mean that other threads will have to repeat the work of re-sorting the parameters, because their borrows were blocking the saving of the cache, but this will not cause failures. The methods on `ParameterTable` that invalidate the cache all require a mutable borrow on the table itself. This makes it impossible for an immutable borrow to exist simultaneously on the cache, so these methods should always succeed to acquire the cache lock to invalidate it. * Use `RefCell::get_mut` where possible In several cases, the previous code was using the runtime-checked `RefCell::borrow_mut` in locations that can be statically proven to be safe to take the mutable reference. Using the correct function for this makes the logic clearer (as well as technically removing a small amount of runtime overhead). * Use `OnceCell` instead of `RefCell` `OnceCell` has less runtime checking than `RefCell` (only whether it is initialised or not, which is an `Option` check), and better represents the dynamic extensions to the borrow checker that we actually need for the caching in this method. All methods that can invalidate the cache all necessarily take `&mut ParameterTable` already, since they will modify Rust-space data. A `OnceCell` can be deinitialised through a mutable reference, so this is fine. The only reason a `&ParameterTable` method would need to mutate the cache is to create it, which is the allowed set of `OnceCell` operations. (cherry picked from commit 2d3db9a) Co-authored-by: Jake Lishman <[email protected]>
Summary
QuantumCircuit.parametersis logically a read-only operation onQuantumCircuit. For efficiency in multiple calls toassign_parameters, we actually cache the sort order of the internalParameterTableon access. This is purely a caching effect, and should not leak out to users.The previous implementation took a Rust-space mutable borrow out in order to (potentially) mutate the cache. This caused problems if multiple Python threads attempted to call
assign_parameterssimultaneously; it was possible for one thread to give up the GIL during its initial call toCircuitData::copy(so an immutable borrow was still live), allowing another thread to continue on to the getterCircuitData::get_parameters, which required a mutable borrow, which failed due to the paused thread incopy.This moves the cache into a
RefCell, allowing the parameter getters to take an immutable borrow as the receiver. We now write the cache out only if we can take the mutable borrow out necessary. This can mean that other threads will have to repeat the work of re-sorting the parameters, because their borrows were blocking the saving of the cache, but this will not cause failures.The methods on
ParameterTablethat invalidate the cache all require a mutable borrow on the table itself. This makes it impossible for an immutable borrow to exist simultaneously on the cache, so these methods should always succeed to acquire the cache lock to invalidate it.Details and comments
This won't be the last time we have problems with Python threading letting people see Rust reject conditions that allow data races. This particular case isn't the user's fault, though -
QuantumCircuit.parametersshouldn't need a mutable borrow, and should be thread-safe to be combined with other thread-safe methods onQuantumCircuit. The issue was an internal caching detail, and that's something Qiskit should help.An example script that produced errors before:
The thread workers in this example aren't doing anything wrong;
assign_parameters(inplace=False)should be safe to run in multiple threads. Before the Rust-space work, Python would silently ignore the potential data race, and the GIL would have prevented anything catastrophic happening (though multiple threads might have separately calculated the sort order).In that example, the
QuantumCircuit.copycall withinQuantumCircuit.assign_parameterstakes an immutable borrow in Rust space, but the Rust space method can become interrupted part way through, while copying theMeasureinstructions (since this requires yielding control to the Python interpreter to runMeasure.copy). This allows another thread to progress toQuantumCircuit.parameters, which needed a mutable borrow, which could not be taken out due to the other paused thread.