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

implement transitive closure computation more efficienctly using differential-dataflow #6

Closed
nikomatsakis opened this issue May 1, 2018 · 5 comments

Comments

@nikomatsakis
Copy link
Contributor

The current naive transitive closure implementation is something that @frankmcsherry explicitly warned me against doing:

https://github.com/rust-lang-nursery/borrow-check/blob/b94f91797945c39b70552a79c91fb17d45bf9118/src/output/timely.rs#L78-L84

There are ways to make this more efficient. For example, we could compute the SCCs directly, perhaps along the lines of the scc example from the differential-dataflow repo.

I've not really thought too hard about what this means. Keep in mind the reason that we compute TC in the first place, described in #5.

@KiChjang
Copy link
Member

KiChjang commented May 4, 2018

I'm going to be taking a look at this.

@KiChjang KiChjang self-assigned this May 4, 2018
@frankmcsherry
Copy link

I'm out from under the deadlines, and available to help in whichever way is best. From recent measurements, we had at least one case where a 360s transitive closure computation dropped down to about 250ms using SCC, though this was an exceptional case (the entire graph was strongly connected, so the result set dropped from 100M to 10K).

@KiChjang
Copy link
Member

KiChjang commented May 4, 2018

I'm not sure if SCC is the right fit here, because subset relationships are mostly acyclic, except for the cases where some regions are required to be equivalent with each other. We're not quite interested in the connectedness of the graph, but rather the paths between two vertices (i.e. is this region R1 a subset of the region Rn?). In particular, this path gets used in computing requires2, as outlined in Niko's blog post.

@KiChjang KiChjang removed their assignment May 12, 2018
@KiChjang
Copy link
Member

Doesn't feel like I can make a lot of progress in this, so unassigning myself from it

@nikomatsakis
Copy link
Contributor Author

I think I basically did this in #23

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

No branches or pull requests

3 participants