Include files in vfs on demand

[HKalbasi]

fix #12181
fix #11025
fix #10178

I'm not sure if this is in line with vfs abstraction or not, but I think we can start with it.

[HKalbasi]

Is it ok?

[HKalbasi]

I think CI failure is unrelated to this PR. It also happens on #13275

[Veykril]

Ye CI fails because the proc-macro server doesnt support 1.64 which released yesterday

[Veykril]

Sorry that I didn't get back to this. I was pondering about this change a bit(in part because I am still not too familiar with our VFS). I think there is a problem here now though, whenever a path won't be resolvable from a given source root (so not because the file ending isn't tracked but because it might lie outside the source root, think .. segments), this will now subscribe that path over and over again as the source root will keep on never resolving the path.

[HKalbasi]

So the problem is that we may push a file multiple times in watched_entries and it would be solved if we check existence before pushing, right? Would it be only specific to paths outside the source root or would it happen with non existent files inside of source root as well?

[Veykril]

Not quite, what I mean is something like the following scenario, we have a module like foo/src/lib.rs which has an include for include!("../../../some_other/path.rs"). This include's path may lie outside of the sourceroot containing foo. Now with the logic here, we'll try to resolve the path from the include, fail and therefor subscribe it. This will trigger a recalculation of the FileSets and SourceRoots, and afterwards as this new path lies outside the sourceroot of foo, it will end up in another sourceroot so we will try to resubscribe that path again, as the foo sourceroot still cant resolve it.

This is basically #9173. Now the issue here specifically is about fixing non rs-files but the way this is implemented it will try to register files from different sourceroots and fail. So either we try to fix the sourceroot problem as well (which is gonna be very tough because we do not want to break the concept of source roots so this would require a lot of though), or we try to limit subscribing to only unknown files that are still within our current sourceroot, though I don't know how simple that would be either (although I assume that to be simpler).

[HKalbasi]

I found a more fundamental problem. This PR in current state actually doesn't work, since watched_entries and watcher fields in NotifyActor don't do anything. They are either dead code or are for a specific config or usages without LSP. Normally, watching is on the client, and LSP doesn't have a request (or I didn't find) for adding a file to watch list. This makes me wonder, how bad it would be to include all files (with some exceptions, for example target directory) without explicit demand?

[Veykril]

Right .. the reason for that I think is that we load the data of all files in the VFS into memory, so if we add all files, we'll have a lot of memory wasted on unnecessary files. So I think this kind of lazy loading would probably be the best choice, though as you said i dont think we can even make that work with client watching ...

[HKalbasi]

In the last changes, the client will watch all the files, but we will read from disk and store in memory only if the file is in the watched_entries. I also fixed the source root problem and now we won't subscribe if the target path belongs to another FileSet.

[HKalbasi]

@Veykril is this problem now resolved?

[HKalbasi]

@rustbot author

[rustbot]

Error: The feature shortcut is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

[Veykril]

Sorry again for having pushed this to side for so long. I am still not too comfortable with this part of the code :) I have to say, I am not too fond of us asking the client to watch every file now, this will include everything in the target folder, and rustc creates a ton of files (my rather freshly cleaned r-a checkout already has 17k files in target again). That's a lot of unnecessary watches ... and afaik file watching is not necessarily cheap on some systems (though I might be wrong here, happy to be proven wrong).

[bjorn3]

Linux has a very low system wide limit on the amount of inodes you can watch.

[Veykril]

Ye figured, then this isn't an option I would say

[HKalbasi]

I think vscode already watches every file for its own proposes, and it is not unhappy with the target directory (there was a non rust project where vscode warned me about too much file to watch, and I had to manually add something (IIRC node_modules) to files.watcherExclude). And I tested this branch on the rust-analyzer and didn't notice any difference in resource usage, on linux and vscode. It uses 4801 inotify watches with and without this changes (my target directory has 38k items, and I see the target directory in r-a logs if I print Invalidate messages, I don't know how it is possible) and cpu/ram usage is normal. So I think it doesn't change anything for vscode, and watching entire directory of a rust project, even big ones like rustc and r-a, is feasible (because vscode can) but I don't know if there are clients with poor implementation of watching.

[bors]

☔ The latest upstream changes (presumably #14061) made this pull request unmergeable. Please resolve the merge conflicts.

[Veykril]

I am going to close this, I don't think this is the right way to approach this and I'd rather we revisit the VFS/SourceRoot abstraction to see if we can come up with a scheme that works out better here.