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

Recover from cyclic include in lazy-input example. #432

Closed
wants to merge 2 commits into from

Conversation

pervognsen
Copy link
Contributor

A potential fix for file cycles in the lazy-input example. This one I'm much, much less sure about. Conceptually, you should be able to have mutually referential files and cycle breaking should only apply at the summation level. This seems to be an issue with the lazy input approach, so I'm curious of the intended approach here.

In a "normal" program you might handle this with a top-level worklist that is populated as you process files and discover dynamic dependencies. Tri-state recursion (unloaded -> loading -> loaded) with cycle breaking would be an option too. That second option seems more congenial to Salsa's way of doing things. When doing it manually, you might do the cycle breaking by registering a file id when transitioning unloaded -> loading (the first time a file is discovered) and then any subsequent call to load the same file would truncate the cycle by returning the file id. However, that is inherently stateful (e.g. you haven't finished loading the file contents when you register the file id initially) and I'm not sure if it's possible in Salsa.

Instead of pontificating further, I thought I'd just bring up this issue and use this mock fix (not really a fix) as a discussion starter.

@netlify
Copy link

netlify bot commented Feb 3, 2023

👷 Deploy Preview for salsa-rs processing.

Name Link
🔨 Latest commit 5571f9f
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/63dc5578bc1a9c0009e79c47

@netlify
Copy link

netlify bot commented Feb 3, 2023

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit fb131af
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/63dc89505c6645000850dd01

@pervognsen
Copy link
Contributor Author

pervognsen commented Feb 3, 2023

I think I figured out a better solution. We create a new interned struct FileId whose field is a canonicalized PathBuf. Then a bunch of the existing uses of File and ParsedFile are replaced by FileId. For example, the links field in ParsedFile changes from a ParsedFile vector to a FileId vector. The parse function takes a FileId as an argument and returns a ParsedFile. The sum function now takes a FileId as an argument, calls parse to get the ParsedFile from the FileId and then goes from there. All in all, this means that parse is now non-recursive and only sum is recursive, so we can move the cycle recovery from parse to sum.

I have this working and will update the PR once I've cleaned it up a bit more.

@nikomatsakis
Copy link
Member

Going to close this since there has been so much activity! Still relevant though, let's come back to it.

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