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

Optimize Data.Graph.bcc #905

Merged
merged 1 commit into from
Jan 30, 2023
Merged

Optimize Data.Graph.bcc #905

merged 1 commit into from
Jan 30, 2023

Conversation

meooow25
Copy link
Contributor

  • Concat difference lists instead of lists to avoid $O(n^2)$ complexity
  • Avoid the unnecessary tree labelling step
  • Add explanatory comments

I've tried a few variations and chosen the one which seems best.

Benchmarks on GHC 9.2.5:
Before:

  bcc
    n=100,m=1000:     OK (0.92s)
      57.0 μs ± 2.9 μs, 507 KB allocated,  29 B  copied, 216 MB peak memory
    n=100,m=10000:    OK (0.25s)
      254  μs ±  22 μs, 1.8 MB allocated,  11 KB copied, 216 MB peak memory
    n=10000,m=100000: OK (7.64s)
      2.526 s ±  96 ms, 3.8 GB allocated, 2.4 GB copied, 1.5 GB peak memory

After:

  bcc
    n=100,m=1000:       OK (0.92s)
      14.4 μs ± 961 ns,  44 KB allocated,  49 B  copied, 215 MB peak memory, 74% less than baseline
    n=100,m=10000:      OK (0.55s)
      107  μs ± 7.8 μs,  43 KB allocated,  74 B  copied, 215 MB peak memory, 58% less than baseline
    n=10000,m=100000:   OK (0.39s)
      3.79 ms ± 189 μs, 5.8 MB allocated, 584 KB copied, 215 MB peak memory, 99% less than baseline
    n=100000,m=1000000: OK (2.98s)
      193  ms ± 1.3 ms,  59 MB allocated,  59 MB copied, 215 MB peak memory

Fixes #900

@treeowl
Copy link
Contributor

treeowl commented Jan 30, 2023

Could you please rebase this and update the before/after benchmarks (since transposeG has changed)?

* Concat difference lists instead of lists to avoid O(n^2) complexity
* Avoid the unnecessary tree labelling step
* Add explanatory comments
@meooow25
Copy link
Contributor Author

Right, rebase done. But transposeG has no effect on bcc so I doubt the benchmarks will change.

@meooow25
Copy link
Contributor Author

Yes it's pretty much the same.

@treeowl
Copy link
Contributor

treeowl commented Jan 30, 2023

I guess I confused it with a different function that did. Oops.

Copy link
Contributor

@treeowl treeowl left a comment

Choose a reason for hiding this comment

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

The whole thing still seems kind of messy, but the speed improvements are quite convincing. Let's go with it for now, to be sure to release before the cutoff. We can revisit it later.

@treeowl treeowl merged commit 58f3bbd into haskell:master Jan 30, 2023
@meooow25
Copy link
Contributor Author

Sure, happy to implement any suggested improvements to make the code cleaner.

@meooow25 meooow25 deleted the graph-bcc branch January 30, 2023 18:08
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.

Data.Graph.bcc is not efficient
2 participants