Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.28k stars 2.37k forks source link

Move interners from trait to generic structs #13033

Closed jakelishman closed 2 months ago

jakelishman commented 2 months ago

Summary

This rewrites the interner mechanisms be defined in terms of two generic structs (Interner and Interned) that formalise the idea that the interned values are owned versions of a given reference type, and move the logically separate intern-related actions (get a key from the value, and get the value from the key) into separate functions (get and insert, respectively).

This has a few advantages:

  1. we now have both insert and insert_owned methods, which was awkward to add within the trait-based structure. This allows a more efficient path when the owned variant has already necessarily been constructed.

  2. additionally, the standard insert path now takes only a reference type. For large circuits, most intern lookups will, in general, find a pre-existing key, so in situations where the interned value is sufficiently small that it can be within a static allocation (which is the case for almost all qargs and cargs), it's more efficient not to construct the owned type on the heap.

  3. the type of the values retrieved from an interner are no longer indirected through the owned type that's stored. For example, where IndexedInterner<Vec<Qubit>> gave out &Vec<Qubit>s as its lookups, Interner<[Qubit]> returns the more standard &[Qubit], which is only singly indirect rather than double.

The following replacements are made:

  1. The IndexedInterner struct from before is now just called Interner (and internally, it uses an IndexSet rather than manually tracking the indices). Its generic type is related to the references it returns, rather than the owned value it stores, so IndexedInterner<Vec<Qubit>> becomes Interner<[Qubit]>.

  2. The Interner trait is now gone. Everything is just accessed as concrete methods on the Interner struct.

  3. <&IndexedInterner as Interner>::intern (lookup of the value from an interner key) is now called Interner::get.

  4. <&mut IndexedInterner as Interner>::intern (conversion of an owned value to an interner key) is now called Interner::insert_owned.

  5. A new method, Interner::insert, can now be used when one need not have an owned allocation of the storage type; the correct value will be allocated if required (which is expected to be less frequent).

  6. The intern key is no longer called interner::Index, but Interned<T>, where the generic parameter T matches the generic of the Interner<T> that gave out the key.

Details and comments

This is sufficiently generic now that it can probably be used directly for the purposes of #12917, though I didn't make it generic over the key width. It probably wouldn't be too hard to do that too, if we want it very much.

I'm intending to follow this PR with one that makes the key corresponding to the "empty" state constructible without any hash lookup, since we end up needing that quite a lot (think the cargs key when pushing a standard gate). (edit: now #13035.)

qiskit-bot commented 2 months ago

One or more of the following people are relevant to this code:

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 10577094637

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/interner.rs 40 44 90.91%
crates/circuit/src/circuit_data.rs 24 30 80.0%
<!-- Total: 136 146 93.15% -->
Files with Coverage Reduction New Missed Lines %
crates/circuit/src/circuit_data.rs 2 92.18%
crates/qasm2/src/lex.rs 3 92.98%
crates/qasm2/src/parse.rs 6 97.61%
<!-- Total: 11 -->
Totals Coverage Status
Change from base Build 10563485346: 0.01%
Covered Lines: 71667
Relevant Lines: 80320

💛 - Coveralls
kevinhartman commented 2 months ago

Looks good, I'll approve once conflicts are resolved.