Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid ExtraInstructionAttributes allocation on unit="dt" #13078

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

jakelishman
Copy link
Member

Summary

The default value for Instruction.unit is "dt". Previously, the OperationFromPython extraction logic would only suppress allocation of the extra instruction attributes if all the contained fields were None, but None is not actually a valid value of Instruction.unit (which must be a string). This meant that OperationFromPython would always allocate and store extra attributes, even for the default cases. This did not affect standard gates appended using their corresponding QuantumCircuit methods (since no Python-space extraction is performed in that case), but did affect standard calls to append, or anything else that entered from Python space.

This drastically reduces the memory usage of circuits built by append-like methods. Ignoring the inefficiency factor of the heap-allocation implementation, this saves 66 bytes plus small-allocation overhead for 2-byte heap allocations (another 14 bytes on macOS, but will vary depending on the allocator) per standard instruction, which is on the order of 40% memory-usage reduction.

Details and comments

I'm using the same sort of microbenchmarking script I've been using since #12730, but now modified to use append instead of the special methods on QuantumCircuit:

from qiskit.circuit import QuantumCircuit, library

QUBITS = 1000
REPS = 3000

def main_methods():
    qc = QuantumCircuit(QUBITS)
    for _ in range(REPS):
        for q in qc.qubits:
            qc.rz(0.0, q)
        for q in qc.qubits:
            qc.rx(0.0, q)
        for q in qc.qubits:
            qc.rz(0.0, q)
        for a, b in zip(qc.qubits[:-1], qc.qubits[1:]):
            qc.cx(a, b)


def main_append():
    qc = QuantumCircuit(QUBITS)
    rz = library.RZGate(0.0)
    rx = library.RXGate(0.0)
    cx = library.CXGate()
    for _ in range(REPS):
        for q in qc.qubits:
            qc.append(rz, (q,))
        for q in qc.qubits:
            qc.append(rx, (q,))
        for q in qc.qubits:
            qc.append(rz, (q,))
        for qs in zip(qc.qubits[:-1], qc.qubits[1:]):
            qc.append(cx, qs)

The memory usage of main_append for the parent of this PR is approximately 2.3GB on both macOS, and Linux with glibc, whereas with the PR it drops to 1.35GB on macOS and 1.06GB on Linux/glibc. I suspect there's something additional going on in the macOS one, because while the Linux/glibc one drops to match the memory usage of main_methods (as expected), macOS remains 300MB higher. It might have been different Python versions - I used 3.10 on macOS and 3.12 on Linux.

The default value for `Instruction.unit` is `"dt"`.  Previously, the
`OperationFromPython` extraction logic would only suppress allocation of
the extra instruction attributes if all the contained fields were
`None`, but `None` is not actually a valid value of `Instruction.unit`
(which must be a string).  This meant that `OperationFromPython` would
always allocate and store extra attributes, even for the default cases.
This did not affect standard gates appended using their corresponding
`QuantumCircuit` methods (since no Python-space extraction is
performed in that case), but did affect standard calls to `append`, or
anything else that entered from Python space.

This drastically reduces the memory usage of circuits built by
`append`-like methods. Ignoring the inefficiency factor of the
heap-allocation implementation, this saves 66 bytes plus
small-allocation overhead for 2-byte heap allocations (another 14 bytes
on macOS, but will vary depending on the allocator) per standard
instruction, which is on the order of 40% memory-usage reduction.
@jakelishman jakelishman added performance Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository labels Sep 3, 2024
@jakelishman jakelishman added this to the 1.3 beta milestone Sep 3, 2024
@jakelishman jakelishman requested a review from a team as a code owner September 3, 2024 09:46
@qiskit-bot
Copy link
Collaborator

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

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

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10680563760

Details

  • 17 of 26 (65.38%) changed or added relevant lines in 2 files are covered.
  • 31 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.02%) to 89.133%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/circuit_instruction.rs 16 25 64.0%
Files with Coverage Reduction New Missed Lines %
crates/circuit/src/circuit_instruction.rs 1 87.41%
crates/qasm2/src/expr.rs 1 94.02%
crates/accelerate/src/two_qubit_decompose.rs 1 90.84%
crates/circuit/src/dag_circuit.rs 3 88.86%
crates/qasm2/src/lex.rs 7 91.48%
crates/qasm2/src/parse.rs 18 96.69%
Totals Coverage Status
Change from base Build 10679848479: -0.02%
Covered Lines: 71849
Relevant Lines: 80609

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this is a straightforward improvement and good to see the memory overhead reductions in practice.

}

/// Get the Python-space default value for the `unit` field.
pub fn default_unit(py: Python) -> &Bound<PyString> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like I would put an #[inline] on this since the compiler is less likely to inline a public function. But it doesn't really matter in practice, especially since this is the python path so that won't help anything.

@mtreinish mtreinish added this pull request to the merge queue Sep 3, 2024
Merged via the queue into Qiskit:main with commit d3b8b91 Sep 3, 2024
15 checks passed
@jakelishman jakelishman deleted the no-extra-attributes branch September 3, 2024 15:44
sbrandhsn pushed a commit to sbrandhsn/qiskit that referenced this pull request Sep 5, 2024
…13078)

The default value for `Instruction.unit` is `"dt"`.  Previously, the
`OperationFromPython` extraction logic would only suppress allocation of
the extra instruction attributes if all the contained fields were
`None`, but `None` is not actually a valid value of `Instruction.unit`
(which must be a string).  This meant that `OperationFromPython` would
always allocate and store extra attributes, even for the default cases.
This did not affect standard gates appended using their corresponding
`QuantumCircuit` methods (since no Python-space extraction is
performed in that case), but did affect standard calls to `append`, or
anything else that entered from Python space.

This drastically reduces the memory usage of circuits built by
`append`-like methods. Ignoring the inefficiency factor of the
heap-allocation implementation, this saves 66 bytes plus
small-allocation overhead for 2-byte heap allocations (another 14 bytes
on macOS, but will vary depending on the allocator) per standard
instruction, which is on the order of 40% memory-usage reduction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog performance Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop storing default unit="dt" in rust space extra attributes by default
4 participants