Skip to content

Move QPY version regex construction to import time#8235

Merged
mergify[bot] merged 3 commits into
Qiskit:mainfrom
mtreinish:import-re-compile-version-qpy
Jun 24, 2022
Merged

Move QPY version regex construction to import time#8235
mergify[bot] merged 3 commits into
Qiskit:mainfrom
mtreinish:import-re-compile-version-qpy

Conversation

@mtreinish
Copy link
Copy Markdown
Member

Summary

As pointed out by @nkanazawa1989 in #8232 the regex construction added
to the QPY interface functions in #8200 can be a significant portion of
the overall function time especially for smaller inputs. Especially as
we're building it on every call interface function call. To ameliorate
that cost this commit compiles the version regex a single time at the
module level. This adds the overhead to import but it will only be done
once which should be a net improvement in performance.

Details and comments

As pointed out by @nkanazawa1989 in Qiskit#8232 the regex construction added
to the QPY interface functions in Qiskit#8200 can be a significant portion of
the overall function time especially for smaller inputs. Especially as
we're building it on every call interface function call. To ameliorate
that cost this commit compiles the version regex a single time at the
module level. This adds the overhead to import but it will only be done
once which should be a net improvement in performance.
@mtreinish mtreinish added stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge. performance Changelog: None Do not include in the GitHub Release changelog. mod: qpy Related to QPY serialization labels Jun 24, 2022
@mtreinish mtreinish added this to the 0.21 milestone Jun 24, 2022
@mtreinish mtreinish requested review from a team and nkanazawa1989 as code owners June 24, 2022 13:37
@qiskit-bot
Copy link
Copy Markdown
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

jakelishman
jakelishman previously approved these changes Jun 24, 2022
Copy link
Copy Markdown
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Looks good to me - my quick search showed these two instances as the only uses of the regex as well (other than the test suite).

Comment thread qiskit/qpy/interface.py Outdated
@coveralls
Copy link
Copy Markdown

coveralls commented Jun 24, 2022

Pull Request Test Coverage Report for Build 2556896243

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0002%) to 83.956%

Totals Coverage Status
Change from base Build 2556893832: 0.0002%
Covered Lines: 55752
Relevant Lines: 66406

💛 - Coveralls

@mergify mergify Bot merged commit ebf800a into Qiskit:main Jun 24, 2022
@mtreinish mtreinish deleted the import-re-compile-version-qpy branch June 24, 2022 17:31
mergify Bot added a commit that referenced this pull request Jun 24, 2022
* Move QPY version regex construction to import time

As pointed out by @nkanazawa1989 in #8232 the regex construction added
to the QPY interface functions in #8200 can be a significant portion of
the overall function time especially for smaller inputs. Especially as
we're building it on every call interface function call. To ameliorate
that cost this commit compiles the version regex a single time at the
module level. This adds the overhead to import but it will only be done
once which should be a net improvement in performance.

* Fix regex flags usage

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit ebf800a)
mergify Bot added a commit that referenced this pull request Jun 24, 2022
* Move QPY version regex construction to import time

As pointed out by @nkanazawa1989 in #8232 the regex construction added
to the QPY interface functions in #8200 can be a significant portion of
the overall function time especially for smaller inputs. Especially as
we're building it on every call interface function call. To ameliorate
that cost this commit compiles the version regex a single time at the
module level. This adds the overhead to import but it will only be done
once which should be a net improvement in performance.

* Fix regex flags usage

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit ebf800a)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
rathishcholarajan added a commit to rathishcholarajan/qiskit-ibm-runtime that referenced this pull request Jul 5, 2022
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
rathishcholarajan added a commit to Qiskit/qiskit-ibm-runtime that referenced this pull request Jul 5, 2022
* Copy Qiskit/qiskit#8055

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Copy qpy changes from Qiskit/qiskit#8093

Co-authored-by: Jake Lishman <jake@binhbar.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* Copy Qiskit/qiskit#8200

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Copy of Qiskit/qiskit#7300

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

* Copy of Qiskit/qiskit#8235

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Jake Lishman <jake@binhbar.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
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. mod: qpy Related to QPY serialization performance stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants