Skip to content

Revert "BasisTranslator to cache bound target DAG. (#6187)"#6457

Merged
mergify[bot] merged 9 commits intoQiskit:mainfrom
kdk:revert-6156-performance-regression-from-disabling-sympy-evaluate
Jun 10, 2021
Merged

Revert "BasisTranslator to cache bound target DAG. (#6187)"#6457
mergify[bot] merged 9 commits intoQiskit:mainfrom
kdk:revert-6156-performance-regression-from-disabling-sympy-evaluate

Conversation

@kdk
Copy link
Copy Markdown
Member

@kdk kdk commented May 24, 2021

This reverts commit 14f105b.

Summary

Following #6386 (comment) , reverts #6187 as the performance benefits of the cache have been superseded by #6270 .

Details and comments

Noisy local benchmarking numbers on examples from #6187 :

All benchmarks:

       before           after         ratio
     [9e770b21]       [2963412f]
     <master>         <revert-6156-performance-regression-from-disabling-sympy-evaluate>
           failed           failed      n/a  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(14, 'synthesis')
           failed           failed      n/a  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(20, 'synthesis')
           failed           failed      n/a  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(27, 'synthesis')
           failed           failed      n/a  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(8, 'synthesis')
        1.67±0.2s        3.08±0.2s    ~1.84  transpiler_benchmarks.TranspilerBenchSuite.time_compile_from_large_qasm
         37.7±4ms         54.9±7ms    ~1.46  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(2, 'translator')
         58.5±4ms        82.0±10ms    ~1.40  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(3, 'translator')
         197±20ms         252±70ms    ~1.28  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(5, 'translator')
       3.20±0.6ms         3.87±2ms    ~1.21  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(1, 'translator')
          1.73±1s          1.99±1s    ~1.15  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(5, 'synthesis')
         188±90ms        205±200ms     1.09  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(3, 'synthesis')
        64.2±10ms        68.9±10ms     1.07  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(2, 'synthesis')
         670±70ms         718±50ms     1.07  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(8, 'translator')
        2.01±0.2s        2.11±0.2s     1.05  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(14, 'translator')
       3.55±0.06s        3.58±0.1s     1.01  randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile_single_thread([[0]])
        8.46±0.3s        8.38±0.5s     0.99  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(27, 'translator')
       4.65±0.06s        4.35±0.4s     0.93  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(20, 'translator')
        1.71±0.2s        1.58±0.1s     0.92  randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile([[0]])
       1.07±0.05m        59.2±0.5s     0.92  randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile_single_thread([[0, 2], [1]])
        17.9±0.4s        16.1±0.9s    ~0.90  randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile_single_thread([[0, 1]])
         11.9±2ms       9.76±0.8ms    ~0.82  quantum_volume.QuantumVolumeBenchmark.time_ibmq_backend_transpile(1, 'synthesis')
          25.3±3s        19.5±0.3s    ~0.77  randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile([[0, 2], [1]])
          8.84±1s        5.66±0.5s    ~0.64  randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile([[0, 1]])

BENCHMARKS NOT SIGNIFICANTLY CHANGED.

@kdk kdk requested a review from a team as a code owner May 24, 2021 18:58
Copy link
Copy Markdown
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, if the benchmarks show there is little to no difference here I think we're fine. But you're probably gonna have to run black before this can merge.

I do think the regression in the large qasm benchmark is real, since if you look at the input circuit: https://github.com/Qiskit/qiskit/blob/master/test/benchmarks/qasm/test_eoh_qasm.qasm there are a lot of repeated gates. I don't remember the exact source that generated it (besides it being an eoh circuit) but it might have been transpiled before the qasm was saved.

@mtreinish mtreinish added automerge Changelog: None Do not include in the GitHub Release changelog. labels Jun 7, 2021
@mergify mergify Bot merged commit 0ef110c into Qiskit:main Jun 10, 2021
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 the GitHub Release changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants