Skip to content

Commit ef98c98

Browse files
committed
Move ParameterVector to Rust
`ParameterVector` was previously a pure-Python concept, even though the Rust-space `Symbol` necessarily had to track an internal backreference to a vector Python object in order to convert itself to Python later. This hidden dependence on the Python interpreter was the root cause of a panicking bug in Qiskit 2.4[^1]. All `ParameterVectorElement`s in Python space have names and UUIDs that are pure functions of the underlying vector and the element's index. Now that this information is available in Rust space, it is far more efficient to simply not store all this derived information, but calculate it on the fly, as needed. This motivated the change in `Symbol` to an `enum`: the logic was _already_ handling the two cases of "standalone" and "element", but this formally separates them to avoid storing unnecessary data. Doing this alone is already a performance benefit: construction of a `list(ParameterVector("a", 10_000))` (to ensure all the same Python-space objects are created) went from 7.7ms to 1.4ms on my machine (~5x speedup). This work here is far more aggressive at threading through shared `Arc` references of both `Symbol` and `SymbolVector`. The `Arc<SymbolVector>` is necessary for correctness in Python space, not just performance: we must have `all(el.vector == els[0].vector for el in els)`, and we don't want to have to cache more Python objects within `Symbol` to achieve that. Unfortunately, `ParameterVector` never implemented true equality (only the default referential equality that Python uses to make mutable-state objects safely hashable by default), so we certainly need some other stable reference that can be shared. The previous Python-space constructions of `PyParameter` and `PyParameterVectorElement` had not been done entirely correctly with respect to subclassing; the same (cloned) `Symbol` was present in multiple places, which was two more allocations than necessary (most places in Rust use `Arc<Symbol>`, which can be near-freely cloned). This corrects it so that `PyParameterVectorElement` is now just a simple marker type, and the majority of the Rust logic needn't concern itself with it at all. The `AtomicUsize` used for the length of `SymbolVector` is to support the unfortunate `ParameterVector.resize` operation from Python space. The atomic operation makes the whole logic safe from data races, but does nothing to solve the potential race condition of two places querying a vector its length concurrently and getting different answers, or the problem that shortening the vector can invalidate already-held references to `ParameterVectorElement`s or `Symbol`s (a problem that was pre-existing in Python). The `resize` in-place method is fairly fundamentally unsound for these reasons, and we should consider removing it; it's only used in the deprecated `NLocal` subclass of `BlueprintCircuit`, and that use can probably be replaced by a simple lazier construction of the vector, or a complete object replacement rather than mutation. [^1]: 16c8088: Fix panic in `RemoveIdentityEquivalent` with `ParameterVector` global phase (gh-16054)
1 parent 8e4b783 commit ef98c98

20 files changed

Lines changed: 611 additions & 690 deletions

File tree

crates/cext/src/param.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ pub unsafe extern "C" fn qk_param_new_symbol(name: *const c_char) -> *mut Param
4747
// Per documentation, the name cannot be empty.
4848
panic!("Invalid empty name.");
4949
} else {
50-
let symbol = Symbol::new(name, None, None);
50+
let symbol = Symbol::standalone(name.to_owned(), None);
5151
let expr = ParameterExpression::from_symbol(symbol);
5252
let param = Param::ParameterExpression(Arc::new(expr));
5353
Box::into_raw(Box::new(param))

crates/cext/src/transpiler/target.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,7 @@ impl TargetEntry {
509509
.map(|i| {
510510
let op_name = operation.name();
511511
Param::ParameterExpression(Arc::new(ParameterExpression::from_symbol(
512-
Symbol::new(format!("{op_name}_param_{i}").as_str(), None, None),
512+
Symbol::standalone(format!("{op_name}_param_{i}"), None),
513513
)))
514514
})
515515
.collect(),

crates/circuit/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,7 @@ pub fn circuit(m: &Bound<PyModule>) -> PyResult<()> {
240240
m.add_class::<parameter::parameter_expression::PyParameterExpression>()?;
241241
m.add_class::<parameter::parameter_expression::PyParameter>()?;
242242
m.add_class::<parameter::parameter_expression::PyParameterVectorElement>()?;
243+
m.add_class::<parameter::parameter_expression::PyParameterVector>()?;
243244
m.add_class::<parameter::parameter_expression::OpCode>()?;
244245
m.add_class::<parameter::parameter_expression::OPReplay>()?;
245246
let classical_mod = PyModule::new(m.py(), "classical")?;

crates/circuit/src/operations.rs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -146,11 +146,7 @@ impl Param {
146146
.getattr(parameters_attr)?
147147
.try_iter()?
148148
.map(|elem| {
149-
let elem = elem?;
150-
let py_param_bound = elem.cast::<PyParameter>()?;
151-
let py_param = py_param_bound.borrow();
152-
let symbol = py_param.symbol();
153-
Ok(symbol.clone())
149+
Ok(Symbol::clone(&elem?.cast_into::<PyParameter>()?.borrow().0))
154150
})
155151
.collect::<PyResult<_>>()?;
156152
Ok(Box::new(collected.into_iter()))

0 commit comments

Comments
 (0)