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

Fix order of do-while loop in transpile optimisation #7794

Merged
merged 2 commits into from
Mar 21, 2022

Conversation

jakelishman
Copy link
Member

Summary

The previous check would always run the body of the optimisation loop
once after the fixed point had been achieved. As well as being
unnecessary, this also made it possible in theory for the optimisation
loop to "unfix" the point, although hopefully our optimisations on that
front would never increase the number of operations.

Instead, we now do the depth/size scans ahead of the loop, then move the
checking condition to the end. In the best-case scenario for
optimisation (the circuit is already optimised) this would be a 2x
speedup of the optimisation loop for free, but more likely it will
manifest itself as a ~20% or so speedup.

Details and comments

A smallish subset of the transpiler benchmarks:

       before           after         ratio
     [592b9acb]       [4977c3a7]
     <main>           <fix-optimisation-condition>
-      2.78±0.03s          2.28±0s     0.82  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_qv_14_x_14(3)
-         519±5ms         426±10ms     0.82  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_from_large_qasm(1)
-      1.67±0.03s        1.36±0.1s     0.81  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_qv_14_x_14(2)
-       1.20±0.4s          947±2ms     0.79  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_qv_14_x_14(1)
-        659±20ms         517±20ms     0.78  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_from_large_qasm(2)
-         840±3ms          653±8ms     0.78  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_from_large_qasm_backend_with_prop(1)
-         663±8ms          494±5ms     0.75  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_from_large_qasm_backend_with_prop(2)

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

This should ameliorate the performance regressions caused by #7542. Note though that performance regressions there were expected and should have come with an accompanying improvement in circuit size, which unfortunately wasn't added to the benchmark suite ahead of time.

The previous check would always run the body of the optimisation loop
once after the fixed point had been achieved.  As well as being
unnecessary, this also made it possible in theory for the optimisation
loop to "unfix" the point, although hopefully our optimisations on that
front would never increase the number of operations.

Instead, we now do the depth/size scans ahead of the loop, then move the
checking condition to the end.  In the best-case scenario for
optimisation (the circuit is already optimised) this would be a 2x
speedup of the optimisation loop for free, but more likely it will
manifest itself as a ~20% or so speedup.
@jakelishman jakelishman requested a review from a team as a code owner March 21, 2022 11:54
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 makes sense to me. There is no reason to run the check at the beginning and end of each loop iteration. Since it will duplicate the same result between the end of the previous iteration and the start of the next.

@coveralls
Copy link

coveralls commented Mar 21, 2022

Pull Request Test Coverage Report for Build 2016183375

  • 8 of 8 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 83.52%

Totals Coverage Status
Change from base Build 2016046325: 0.001%
Covered Lines: 52541
Relevant Lines: 62908

💛 - Coveralls

@mergify mergify bot merged commit 420c92c into Qiskit:main Mar 21, 2022
@jakelishman jakelishman deleted the fix-optimisation-condition branch March 21, 2022 14:34
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants