Cache lexicographical topological sort key in DAGNode #4040
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit caches the string sort key used for lexicographical topological sort as an attribute in DAGNode objects. Currently when retworkx's lexicographical topological sort call is called it needs to call the python callable provided to get a string used for sorting. Prior to this commit about 70% of the cumulative time spent in the lexicographical topological sort function is spent in python. This is because everytime it is sorting a new DAGNode it has to call str(x.qargs) which spends all that time constructing the string including a dict.get() and bit and register repr() calls, etc. To avoid this unecessary overhead at run time this commit adds a sort_key attribute to the DAGNode class which is set during __init__() and updates to the qargs property. Then the DAGCircuit lexicographical topological sort wrapper method changes the callable to use this attribute. Doing this eliminates all the overhead from calling back to python because it simply becomes attribute access.
|
To see the impact this makes, profiling a level 1 transpile of a QV 100x100 model circuit the lexicographical topological sort profile without this PR looks like:
After applying this PR most of the Python overhead is eliminated and it becomes:
|
ajavadia
approved these changes
Mar 29, 2020
faisaldebouni
pushed a commit
to faisaldebouni/qiskit-terra
that referenced
this pull request
Aug 5, 2020
This commit caches the string sort key used for lexicographical topological sort as an attribute in DAGNode objects. Currently when retworkx's lexicographical topological sort call is called it needs to call the python callable provided to get a string used for sorting. Prior to this commit about 70% of the cumulative time spent in the lexicographical topological sort function is spent in python. This is because everytime it is sorting a new DAGNode it has to call str(x.qargs) which spends all that time constructing the string including a dict.get() and bit and register repr() calls, etc. To avoid this unecessary overhead at run time this commit adds a sort_key attribute to the DAGNode class which is set during __init__() and updates to the qargs property. Then the DAGCircuit lexicographical topological sort wrapper method changes the callable to use this attribute. Doing this eliminates all the overhead from calling back to python because it simply becomes attribute access.
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Jan 26, 2025
Prior to having the DAGCircuit in rust the `sort_key` attribute was added as a cache to speed up the lexicographical topological sort. Prior to having this attribute the topological sort method would compute the sort key as a string on the fly which ended up being a large bottleneck in transpilation (see: Qiskit#4040 for more details). However, since migrating the DAGCircuit implementation to Rust this sort_key attribute isn't needed anymore because we call the rustworkx-core function with a tuple of bit objects instead of a string. The `sort_key` atribute was left on in place for backwards compatibility (it shouldn't have been public, this was a mistake in Qiskit#4040) and when we create a python DAGNode object that will be returned to Python the creation of the sort key is unnecessary overhead (it takes roughly 1% of profile time to format the sort_key string during transpilation). Since nothing in DAGCircuit is actually using it this commit removes it to save the CPU cycles creating the string on each dag creation. We will need to add a deprecation to 1.4.0 to mark this removal for 2.0.0 since this was missed in 1.3.0.
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Jan 26, 2025
Prior to having the DAGCircuit in rust the `sort_key` attribute was added as a cache to speed up the lexicographical topological sort. Prior to having this attribute the topological sort method would compute the sort key as a string on the fly which ended up being a large bottleneck in transpilation (see: Qiskit#4040 for more details). However, since migrating the DAGCircuit implementation to Rust this sort_key attribute isn't needed anymore because we call the rustworkx-core function with a tuple of bit objects instead of a string. The `sort_key` atribute was left on in place for backwards compatibility (it shouldn't have been public, this was a mistake in Qiskit#4040) and when we create a python DAGNode object that will be returned to Python the creation of the sort key is unnecessary overhead (it takes roughly 1% of profile time to format the sort_key string during transpilation). Since nothing in DAGCircuit is actually using it this commit removes it to save the CPU cycles creating the string on each dag creation. We will need to add a deprecation to 1.4.0 to mark this removal for 2.0.0 since this was missed in 1.3.0.
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Jan 27, 2025
Prior to having the DAGCircuit in rust the sort_key attribute was added as a cache to speed up the lexicographical topological sort. Prior to having this attribute the topological sort method would compute the sort key as a string on the fly which ended up being a large bottleneck in transpilation (see: Qiskit#4040 for more details). However, since migrating the DAGCircuit implementation to Rust this sort_key attribute isn't needed anymore because we call the rustworkx-core function with a tuple of bit objects instead of a string. The sort_key atribute was left on in place for backwards compatibility (it shouldn't have been public, this was a mistake in Qiskit#4040) and when we create a python DAGNode object that will be returned to Python the creation of the sort key is unnecessary overhead (it takes roughly 1% of profile time to format the sort_key string during transpilation). Since nothing in DAGCircuit is actually using it this commit removes it to save the CPU cycles creating the string on each dag creation. This attribute is removed in Qiskit 2.0.0 in Qiskit#13736
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 27, 2025
Prior to having the DAGCircuit in rust the sort_key attribute was added as a cache to speed up the lexicographical topological sort. Prior to having this attribute the topological sort method would compute the sort key as a string on the fly which ended up being a large bottleneck in transpilation (see: #4040 for more details). However, since migrating the DAGCircuit implementation to Rust this sort_key attribute isn't needed anymore because we call the rustworkx-core function with a tuple of bit objects instead of a string. The sort_key atribute was left on in place for backwards compatibility (it shouldn't have been public, this was a mistake in #4040) and when we create a python DAGNode object that will be returned to Python the creation of the sort key is unnecessary overhead (it takes roughly 1% of profile time to format the sort_key string during transpilation). Since nothing in DAGCircuit is actually using it this commit removes it to save the CPU cycles creating the string on each dag creation. This attribute is removed in Qiskit 2.0.0 in #13736
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 27, 2025
* Remove sort_key attribute from DAGNode Prior to having the DAGCircuit in rust the `sort_key` attribute was added as a cache to speed up the lexicographical topological sort. Prior to having this attribute the topological sort method would compute the sort key as a string on the fly which ended up being a large bottleneck in transpilation (see: #4040 for more details). However, since migrating the DAGCircuit implementation to Rust this sort_key attribute isn't needed anymore because we call the rustworkx-core function with a tuple of bit objects instead of a string. The `sort_key` atribute was left on in place for backwards compatibility (it shouldn't have been public, this was a mistake in #4040) and when we create a python DAGNode object that will be returned to Python the creation of the sort key is unnecessary overhead (it takes roughly 1% of profile time to format the sort_key string during transpilation). Since nothing in DAGCircuit is actually using it this commit removes it to save the CPU cycles creating the string on each dag creation. We will need to add a deprecation to 1.4.0 to mark this removal for 2.0.0 since this was missed in 1.3.0. * Simplify star pre-routing sort_key logic
emilkovacev
pushed a commit
to emilkovacev/qiskit
that referenced
this pull request
Feb 7, 2025
* Remove sort_key attribute from DAGNode Prior to having the DAGCircuit in rust the `sort_key` attribute was added as a cache to speed up the lexicographical topological sort. Prior to having this attribute the topological sort method would compute the sort key as a string on the fly which ended up being a large bottleneck in transpilation (see: Qiskit#4040 for more details). However, since migrating the DAGCircuit implementation to Rust this sort_key attribute isn't needed anymore because we call the rustworkx-core function with a tuple of bit objects instead of a string. The `sort_key` atribute was left on in place for backwards compatibility (it shouldn't have been public, this was a mistake in Qiskit#4040) and when we create a python DAGNode object that will be returned to Python the creation of the sort key is unnecessary overhead (it takes roughly 1% of profile time to format the sort_key string during transpilation). Since nothing in DAGCircuit is actually using it this commit removes it to save the CPU cycles creating the string on each dag creation. We will need to add a deprecation to 1.4.0 to mark this removal for 2.0.0 since this was missed in 1.3.0. * Simplify star pre-routing sort_key logic
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This commit caches the string sort key used for lexicographical
topological sort as an attribute in DAGNode objects. Currently when
retworkx's lexicographical topological sort call is called it needs to
call the python callable provided to get a string used for sorting.
Prior to this commit about ~70% of the cumulative time spent in the
lexicographical topological sort function is spent in python. This is
because every time it is sorting a DAGNode it has to call
str(x.qargs) which spends all that time constructing the string
including a dict.get() and bit and register repr() calls, etc. To avoid
this unnecessary overhead at run time this commit adds a sort_key
attribute to the DAGNode class which is set during
__init__()andupdates to the qargs property. Then the DAGCircuit lexicographical
topological sort wrapper method changes the callable to use this
attribute. Doing this eliminates all the overhead from calling back to
python because it simply becomes attribute access.
Details and comments