Localise py_op caching data in RefCell

[jakelishman]

Summary

This localises the caching concerns of the PackedInstruction::py_op field into a method unpack_py_op, which can now update the cache through an immutable reference (if no other immutable references are taken out). Having the new method to encapsulate the cache_pyops feature simplifies the large #[cfg(feature = "cache_pyop")] gates.

Details and comments

Locally, this had no impact for me on runtime performance for the benchmarks of #12459 if (on macOS) I set the MallocMediumZone=0 environment variable, but I did see a reduction in performance without that set. That said, setting it also caused a sizeable improvement in the benchmarks without this PR as well.

This will make memory usage a bit worse while the pygate caching is in place (the RefCell makes the cost 2 pointers rather than one), but since we're roughly hoping to remove that before 1.2, it's probably worth doing. This commit makes it much harder to forget to set the cache (which a few places in the existing code were doing), and means that the APIs (&self vs &mut self) don't need to change within the feature flag, which will make ripping the cache out easier in the future. edit: if necessary, we can trade off the memory usage against a shade more runtime cost by boxing the RefCell, which should be ok since performance-critical Qiskit code largely shouldn't be using the caching mechanisms by the time of the 1.2 release, but if the caching is still enabled in release builds, then the memory costs would be paid by everyone.

Will clash with #12593 (most of its changes are already made locally here too) but I can sort that out easily.

[qiskit-bot]

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

• @Eric-Arellano
• @Qiskit/terra-core
• @kevinhartman
• @mtreinish

[coveralls]

Pull Request Test Coverage Report for Build 9552158178

Details

• 63 of 63 (100.0%) changed or added relevant lines in 2 files are covered.
• 19 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.006%) to 89.735%

---

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	4	92.62%
crates/qasm2/src/parse.rs	6	97.15%
crates/circuit/src/circuit_data.rs	9	90.94%

Totals
Change from base Build 9551377748:	-0.006%
Covered Lines:	63341
Relevant Lines:	70587

---

💛 - Coveralls

[kevinhartman]

This LGTM.

I like the idea of using the RefCell for interior mutability to avoid requiring &mut refs when we'll be doing away with the cache stuff later on.

I'm not hitting "merge" in case @mtreinish wants to take a look first, since this is an extension of his operation oxidation work.

[jlapeyre]

This looks like it does: 1) refactor a lot of code into unpack_py_op. and 2) Use RefCell to hide the mutability of a reference.
When caching the python op is removed there will be no need for a mutable ref.

I'm curious if it is possible to get the same refactoring benefit without using RefCell, rather continuing to use a plain vanilla mutable ref. Does the part about making it harder to forget to update the cache depend only on the refactoring?

Since caching the Python op is supposed to be a bridge anyway, and there are not dramatic performance differences, maybe it's not a big deal whether RefCell is used or not. I do like the idea of boxing the RefCell.

[jakelishman]

John: in theory this can be done without the RefCell, just that then everywhere that handles PackedInstruction needs to have a mutable reference to it, not just a base reference. Since PackedInstruction is the data-at-rest format from within the CircuitData struct, that means that methods on CircuitData would need to take that by mutable reference as well, etc, so it proliferates.

About the refactoring: reducing the duplicated code in the #[cfg(feature = "cache_pygates")] down to single expressions was actually the original purpose of this PR, but in doing that I realised that there were places that were forgetting to update the cache (I think CircuitData::foreach_op was one?) and that there were some functions where the split was that the cache_pygates version needed to take a mutable reference where the base method took an immutable one.

[jakelishman]

With the same principles of moving fast and undoing if necessary, I'll merge this now to avoid further merge conflicts, since it may well get touched up as part of #12622 anyway.