Skip to content

Improve performance of Graph.transitive_closure() - #1014

Merged
ahal merged 2 commits into
taskcluster:mainfrom
ahal:ahal/xvuotksxtnmq
Jul 29, 2026
Merged

Improve performance of Graph.transitive_closure()#1014
ahal merged 2 commits into
taskcluster:mainfrom
ahal:ahal/xvuotksxtnmq

Conversation

@ahal

@ahal ahal commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@ahal ahal self-assigned this Jul 29, 2026
@ahal
ahal requested a review from a team as a code owner July 29, 2026 15:06
@ahal
ahal requested a review from bhearsum July 29, 2026 15:06
@ahal
ahal marked this pull request as draft July 29, 2026 15:12
@codspeed-hq

codspeed-hq Bot commented Jul 29, 2026

Copy link
Copy Markdown

Merging this PR will regress 1 benchmark

⚡ 3 improved benchmarks
❌ 1 regressed benchmark
✅ 20 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
test_transitive_closure[fan] 19 ms 35.3 ms -46.05%
test_transitive_closure[linear] 101,008.4 ms 40.2 ms ×2,500
test_transitive_closure[diamond] 153.6 s 2 s ×77
test_transitive_closure[btree] 46.3 ms 26.3 ms +76.04%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ahal:ahal/xvuotksxtnmq (99a789c) with main (d7efa4b)1

Open in CodSpeed

Footnotes

  1. No successful run was found on main (305a07f) during the generation of this report, so d7efa4b was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@ahal
ahal marked this pull request as ready for review July 29, 2026 15:20
Previously we were looping over all edges for each node in the graph.
Now we build a dict, making the traversal linear rather than quadratic.
@ahal
ahal force-pushed the ahal/xvuotksxtnmq branch from 9fd9097 to 99a789c Compare July 29, 2026 16:01
@ahal

ahal commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

I don't think there's a way to not regress the fan test. This is a tradeoff, but should be worth it as long as the real graphs have linear / diamond segments in them (which Gecko does).

Ultimately none of this is going to have any measurable impact on real world graphs.. but this does at least make the codspeed task fast(ish) again.

I think we should take it.

@bhearsum

Copy link
Copy Markdown
Contributor

If you could, maybe a quick before/after test with at least one gecko graph as a sanity check would be in order before this lands?

@ahal

ahal commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

If you could, maybe a quick before/after test with at least one gecko graph as a sanity check would be in order before this lands?

There's too much noise to tell, especially due to that one 70s outlier. Seems like there's not a regression at least (as expected)

Before:

$ hyperfine "./mach taskgraph morphed" -w 1 -r 5
Benchmark 1: ./mach taskgraph morphed
  Time (mean ± σ):     51.263 s ± 11.194 s    [User: 114.538 s, System: 37.939 s]
  Range (min … max):   38.233 s … 69.037 s    5 runs

After:

$ hyperfine "./mach taskgraph morphed" -w 1 -r 5
Benchmark 1: ./mach taskgraph morphed
  Time (mean ± σ):     44.317 s ±  6.868 s    [User: 116.071 s, System: 34.583 s]
  Range (min … max):   34.272 s … 51.743 s    5 runs

@ahal
ahal merged commit 8169bc2 into taskcluster:main Jul 29, 2026
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants