qiskit
qiskit copied to clipboard
Qiskit
Add no-hash-lookup way to retrieve the default interned value
Summary
This makes it possible to do Interner::get_default() without any value, in order to retrieve a key pointing to the default allocation without making any hash lookups. While the hashing and equality check of the default allocation is typically very cheap (like the empty slice), acquiring it still generally required a function call, which often needed to be paid frequently.
Details and comments
Built on top of #13033, so depends on it.
Pull Request Test Coverage Report for Build 10623649976
Warning: 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.
- For more information on this, see Tracking coverage changes with pull request builds.
- To avoid this issue with future PRs, see these Recommended CI Configurations.
- For a quick fix, rebase this PR at GitHub. Your next report should be accurate.
Details
- 11 of 11 (100.0%) changed or added relevant lines in 2 files are covered.
- 263 unchanged lines in 4 files lost coverage.
- Overall coverage decreased (-0.04%) to 89.176%
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| crates/accelerate/src/two_qubit_decompose.rs | 1 | 90.82% |
| crates/qasm2/src/lex.rs | 3 | 91.48% |
| crates/qasm2/src/parse.rs | 18 | 96.69% |
| crates/circuit/src/dag_circuit.rs | 241 | 89.02% |
| <!-- | Total: | 263 |
| Totals | |
|---|---|
| Change from base Build 10579458520: | -0.04% |
| Covered Lines: | 71633 |
| Relevant Lines: | 80328 |
💛 - Coveralls
One or more of the following people are relevant to this code:
@Qiskit/terra-core@kevinhartman@mtreinish
Another API alternative here is to add an implementation of Default / explicit method to Interned<T> where T: ?Sized + ToOwned, <T as ToOwned>::Owned: Default that constructs the same struct. For now I just put the method on the interner for the symmetry with get, and because I wasn't entirely sure how the type inference would play out if it was on Interned (I didn't want the caller to have to turbo-fish the default constructor). Feels like it could easily go either way (or both ways).
Another API alternative here is to add an implementation of Default / explicit method to Interned<T> where T: ?Sized + ToOwned, <T as ToOwned>::Owned: Default that constructs the same struct. For now I just put the method on the interner for the symmetry with get, and because I wasn't entirely sure how the type inference would play out if it was on Interned (I didn't want the caller to have to turbo-fish the default constructor). Feels like it could easily go either way (or both ways).
I see what you mean. I think having both could be nice, but the explicit method seems like it'd be easier for folks to find, which may result in us seeing less of .insert(&[]) in new PRs.
What was the reason not to impl Default itself for Interned<T> with constraints on T?
Otherwise, looks good.
I didn't think super hard about it, I think my default API position is just to define concrete methods rather than hide new functionality in trait implementations.
Thinking more now, I'd say that "the interned key of the default value" is not necessarily the same thing as the "default interned key", but that said: maybe my position on the trait is skewed, because imo integer types shouldn't have a default, and neither should an interner key. Judging by the API choices in both Python and Rust, though, I'm in the minority there (e.g. Python's int() and Rust's impls of Default for integers).
I didn't think super hard about it, I think my default API position is just to define concrete methods rather than hide new functionality in trait implementations.
I think in this case, I would actually rather we just have the get_default like you had it before. If you want to also implement Default, I'd be on board.
Thinking more now, I'd say that "the interned key of the default value" is not necessarily the same thing as the "default interned key"
Hmm. It's late in the night so I'm probably not picking up on the subtly, but wouldn't that break the premise of us using Default as the trait bound for this whole thing anyhow? I guess I can see what you mean, but I'd think it'd follow that the Interner should decide what its keys are, so in that case we should probably not let Interned structs be constructed on their own.
I'm fine to remove it too, just let me know. If you want me to turn it into an actual Default impl that's fine too, since the explicit method will still exist (just be called default).
I'd say that Interned and Interner are so intrinsically linked that it makes little difference who decides what here - the two structs absolutely have to know how each other work in order not to violate each other's contracts. The "default" key being index 0 is fairly core to how the system works, and any risk of Interned and Interner getting out of sync on that numbering is the same as it was before between the Interner::new and Interner::get_default methods. I don't think there's a new risk by having the Interned method because I forced the creation routines to be consistent by having get_default call it - the risk of divergence with the Interner constructor remains the same (and personally I'm not worried about it).
I'm not worried either 🙂. But tl;dr, I think you should just remove Interned::of_default for now.
I don't think there's a new risk by having the Interned method because I forced the creation routines to be consistent by having get_default call it
What I meant was more that we're locking ourselves out of creating default Interned keys statefully (in the context of an Interner) by having a method to do it directly without an Interner. Whereas your get_default method on Interner takes a &self, so the default it produces could make use of some internal state. Does that matter? Probably not, but I don't see a reason to have both Interner::get_default and Interned::of_default (it doesn't feel more discoverable from an API perspective), and get_default seems safer and more in line with the idea that we're asking an Interner for a local reference to something within its jurisdiction.
Ok, it's gone again in b96698b381.
(Fwiw, given none of this Rust stuff is in any way public, I'm not really worried about locking ourselves out of anything - this PR's parent just changed the entire Interner interface, for example. Keeping the constructor and the index inside the Interned type private from ourselves stops us from leaking the "the default index is 0" abstraction anywhere - and if people were making assumptions about the indices, we'd have rather larger problems anyway.)