A try on improving sharding dependency resolution efficiency #32
Conversation
|
For reference, currently without this change it takes 2m20s: ❯ python -m cProfile -s cumtime .venv/bin/phirc tests/data/integration/measurement_crosstalk_l=1000.qasm
Processing tests/data/integration/measurement_crosstalk_l=1000.qasm
132896881 function calls (131639750 primitive calls) in 139.752 seconds
Ordered by: cumulative time
ncalls tottime percall cumtime percall filename:lineno(function)
2710/1 0.033 0.000 139.763 139.763 {built-in method builtins.exec}
1 0.012 0.012 139.763 139.763 phirc:1(<module>)
1 0.070 0.070 137.537 137.537 cli.py:20(main)
1 0.001 0.001 94.706 94.706 api.py:21(pytket_to_phir)
1 0.002 0.002 55.318 55.318 sharder.py:51(shard)
7036 0.053 0.000 49.270 0.007 sharder.py:75(_process_command)
7021 30.739 0.004 49.189 0.007 sharder.py:101(_build_shard)
2 0.000 0.000 25.890 12.945 rebaser.py:9(rebase_to_qtm_machine)
4 25.847 6.462 25.854 6.463 {built-in method pytket._tket.passes.apply}
2 0.000 0.000 25.843 12.922 backend.py:175(get_compiled_circuit)
1 0.003 0.003 25.646 25.646 place_and_route.py:8(place_and_route)
1 0.025 0.025 25.457 25.457 shards2ops.py:8(parse_shards_naive)
3012 1.530 0.001 25.426 0.008 shards2ops.py:22(<listcomp>)
10598686 23.895 0.000 23.895 0.000 {method 'issubset' of 'set' objects} |
| self._get_dependencies_recursive(dependencies, shard) | ||
| return dependencies | ||
|
|
||
| def _get_dependencies_recursive(self, accumulator: set[int], shard: Shard) -> None: |
There was a problem hiding this comment.
Maybe I'm overthinking this, but if we're trying to improve performance--then is there any way to replace this with a data-structure like a stack/queue and some sort of while-loop? Or is the overhead of the recursion here minimal?
There was a problem hiding this comment.
Sorry, my comment above may have been confusing, currently the changes in this PR lead to significant overhead and hence it is not ready to be merged. make tests is taking way longer than usual (perhaps because of the issue with recursion that you just identified).
Compare the build times with another recent PR:
There was a problem hiding this comment.
Not 100% sure but since this is clearly not a slam dunk I'm going to close it and try a more significant refactor that should reduce to linear time.
| depends_upon.add(shard.ID) | ||
| # Avoid recalculating for anything previous |
There was a problem hiding this comment.
Seems this avoiding recalculation is the main thing to help performance?
peter-campora
dismissed
their stale review
Whoops, misunderstood conversation around the time taken for that test.
A bit complicated due to some older decisions. If considerable speedup seen, can merge for now until replace with linear-time implementation.