VFS watcher impl #556

vemoo · 2019-01-15T16:58:39Z

Implements #433

For now I ended up referencing my forked notify with the fix until it's merged and released.

There are some things left to do, like what to do on Rescan or Error events, but the rest it's working.

crates/ra_vfs/src/lib.rs

crates/ra_vfs/src/watcher.rs

vemoo · 2019-01-16T17:26:23Z

crates/ra_vfs/src/lib.rs

                root,
                path: path.clone(),
                filter: Box::new(filter),
            };
            res.worker.inp.send(task).unwrap();
+            res.watcher.watch(path).unwrap();


I will add error handling and log a warning for errors.

vemoo · 2019-01-16T18:57:12Z

I was missing the Task:HandleChange that lets the Vfs decide which changes should be loaded. And it was trying to load everything.

After fixing that I'm still seing high CPU usage and the ra_lsp_server sometimes hangs and doesn't close after closing the editor. I will investigate, but I think it's not ready to merge yet.

matklad · 2019-01-19T12:49:32Z

Note that I've had to rewrite the history recently, so expect storms while rebasing: #566 (comment)

Sorry about that: it's definitively my mistake 🙁

vemoo · 2019-01-19T22:39:53Z

Didn't run into any problems when rebasing.

I found out what the issue was. Using the default recursive watching ment also watching the target folder, and during builds there's a lot of activity there. It was so bad that after running cargo build and then cargo clean my system started swapping after ra_lsp_server took all the memory.

I've changed the watching to non recursive and implemented recursive watching taking .gitignore into account, using the ignore crate. I'm not sure if that's the right way, since that wouldn't work on projects that are not on git. Maybe excluding build folders would be better, but I don't know if there's an easy way to do it.

matklad · 2019-01-20T10:24:30Z

Huh, that is getting more complex that I have anticipated...

I think “watch only specific stuff” is the right approach, but we must share that same filtering logic betweeen initial scan and subsequent watching.

Specifically RootFilter is the thing we want to use here:
https://github.com/rust-analyzer/rust-analyzer/blob/11a77d76a9759a24cfbe507550db262e22f830ad/crates/ra_vfs/src/lib.rs#L38

Conceptually, SourceRoot is the set of files in the directory, and RootFilter captures this concept. It currently only filters by .rs extension, but it should filter out /target directory as well. I think it’s fine to filter out target by default.

Using gitignore is also a good idea, but I think it should be read by the clients of vfs and supplied at a construction time as a RootFilter. Perhaps a RootGlob would be a better game even?

vemoo · 2019-01-21T21:26:17Z

Ok I think it's almost there.

I have implemented recursive watching using the RootFilter but without .gitignore for now.

Right now in rust-analyzer there are around 218 roots (between cargo registry and workspace crates) so each time there's a change in a watched dir we iterate all of them to check if it belogs to any of them.
So right know even for changes in the .git folder or node_modules we do that, but when .gitignore is implemented that won't happen because we won't be watching those directories.
Still, I wonder if it's worth it to store the roots in a hierarchy to avoid always iterating on all of them.

I think this should be usable now. Maybe the .gitignore support can be implemented in a separate PR?

matklad · 2019-01-22T10:05:52Z

So right know even for changes in the .git folder or node_modules we do that

I'd just hard-code those for now as well :)

Maybe the .gitignore support can be implemented in a separate PR?

Makes sense to me! I'll also be fine with just hard-coding worst offenders and punting on this until notify 5.0

Still, I wonder if it's worth it to store the roots in a hierarchy to avoid always iterating on all of them.

Sure, long-term we need faster data structures everywhere. Everything in rust-analyzer currently uses a dumb linear scan and recomputed every time. However, I think that before starting with "obvious" optimizations, we need to put some integration profiling infrastructure in place. That is, I'd like to see info like "we've spent 5 CPU seconds on traversing the roots in the last minute of rust-analyzer usage".

crates/ra_vfs/src/io/mod.rs

vemoo · 2019-01-22T18:09:24Z

Ok, I've hardcoded .git and node_modules in any path component aswell.

Sure, long-term we need faster data structures everywhere. Everything in rust-analyzer currently uses a dumb linear scan and recomputed every time. However, I think that before starting with "obvious" optimizations, we need to put some integration profiling infrastructure in place. That is, I'd like to see info like "we've spent 5 CPU seconds on traversing the roots in the last minute of rust-analyzer usage".

Ok, that makes sense. But I think it's related to how ignoring files should be implemented. Using this project as an example there's a .gitignore file at the root level that affets all of the crates. With the current structure all of the RootFilters should know about it. But if there's some hierarchy only the outer root should know about it.

What I'm thinking is a tree with nodes like:

struct Node {
    ignores: Vec<Ignore>,
    path: PathBuf,
    root: Option<VfsRoot>,
    children: Vec<Node>,
}

root is optional to be able to create nodes for common paths like cargo registry or the other ignore files are.

crates/ra_vfs/src/lib.rs

crates/ra_vfs/src/io.rs

matklad · 2019-01-23T18:10:26Z

crates/ra_vfs/src/io.rs

+        root: VfsRoot,
+        path: PathBuf,
+        root_filter: Arc<RootFilter>,
+        nested_roots: Vec<PathBuf>,


I think nested_roots should just be a part of RootFilter, with a more generic name like excluded_dirs.

matklad · 2019-01-23T18:35:24Z

crates/ra_vfs/src/io.rs

+        }
+        Task::HandleChange(change) => {
+            // forward as is because Vfs has to decide if we should load it
+            TaskResult::HandleChange(change)


Hm, this looks like one elaborate control flow! I wonder if we could simplify this... At least when I originally thought about it, I've thought of io as a separate actor which completely encapsulates file system access and gives very "clean" TaskResults outside:

enum TaskResult { BulkLoadRoot { root: VfsRoot, files: Vec<(Path, String)> }, AddSingleFile { root: VfsRoot, path: Path, text: String}, ChangeSingleFile { ... }, RemoveSingleFile { ... }, }

That is, io handles all loading and watching of roots, and vfs itself then decides if it should ignore ChangeSingleFile due to memap or not. Watcher becomes and implementation detail of IO.

I think that probably Worker will need to know the set of all roots for this to work, so perhaps we should encapsulate roots: Arena<VfsRoot, Arc<RootFilter>>, into Roots struct and share it via an Arc between vfs and Worker?

Obviously, I don't know the specifics of notify API very well, so perhaps this wont' work for some reason?

Ok, yes, that sounds better and I think it should work, I will try to implement it tomorrow.

Should Roots struct be in a RwLock? At the moment it can be implemented without it but if the vfs will support adding roots after initialization it will be needed.

Should Roots struct be in a RwLock? At the moment it can be implemented without it but if the vfs will support adding roots after initialization it will be needed.

I'd lean towards sharing an immutable snapshot for starters.

EDIT: that is, just Arc<Roots>, without RwLock. I think adding dynamically reconfigurable roots will be a giant can of worms, which actually might completely change the design here.

This is now implemented.

I had to move the watcher to inside the io workers thread:
https://github.com/rust-analyzer/rust-analyzer/blob/1692a11d09cc3d70dcde10cc896c6f7ebb9eb3c3/crates/ra_vfs/src/io.rs#L50-L51
to have access to the output sender and be able to send the TaskResults directly from the watcher.

I've added excluded_dirs to RootFilter and also
https://github.com/rust-analyzer/rust-analyzer/blob/1692a11d09cc3d70dcde10cc896c6f7ebb9eb3c3/crates/ra_vfs/src/lib.rs#L63
to make using it with walkdir cleaner.

The rest is pretty much as you described it, which is a lot cleaner.

One thing I have found is that WalkDir by default doesn't follow symlinks which means that entry.file_type().is_[dir|file]() and entry.path().is_[dir|file]() won't necessarily match. I'm thinking we should probably follow symlinks but i'm not sure.

Yeah, this now looks superb! I’ll do a manual smoke test tomorrow and merge!

I think it’s ok not to follow simlnks for now

use same channel for scanner and watcher some implementations pending

…d files contents in `io`

use `RootFilter` to filter recursive watches untested

@vemoo

665: Pr 556 r=matklad a=matklad Rebased #556 Thanks @vemoo, now I can change branches without reopening VS Code! Co-authored-by: Bernardo <berublan@gmail.com>

matklad · 2019-01-26T09:21:40Z

Merged in #665!

matklad · 2019-01-30T17:08:33Z

Hm, I still get occasional 100% CPU hangs. Need to investigate this

vemoo · 2019-01-30T22:25:48Z

In my case it happened when watching directories with a lot of notifications, and was accompanied by high memory usage. Maybe try adding logging for the all the notify events and see if there's something unusual there.

vemoo · 2019-02-06T18:34:36Z

I've found something that may be related notify-rs/notify#177

matklad reviewed Jan 15, 2019

View reviewed changes

crates/ra_vfs/src/lib.rs Outdated Show resolved Hide resolved

matklad reviewed Jan 15, 2019

View reviewed changes

crates/ra_vfs/src/watcher.rs Outdated Show resolved Hide resolved

vemoo commented Jan 16, 2019

View reviewed changes

vemoo force-pushed the vfs-watch branch from 4aea6b9 to 11a77d7 Compare January 19, 2019 22:26

matklad reviewed Jan 22, 2019

View reviewed changes

crates/ra_vfs/src/io/mod.rs Outdated Show resolved Hide resolved

JeanMertz reviewed Jan 23, 2019

View reviewed changes

crates/ra_vfs/src/lib.rs Outdated Show resolved Hide resolved

matklad reviewed Jan 23, 2019

View reviewed changes

vemoo added 16 commits January 25, 2019 21:06

initial Watcher impl

e3c9840

complete test

758c339

refator to move all io to io module

6b1917e

use same channel for scanner and watcher some implementations pending

handle watched events filtering in Vfsadd is_overlayedload change…

b7bc748

…d files contents in `io`

reference notify issue

d3a7d49

use notify with fix

b003570

better error handling

c8ebc19

add missing Task::HandleChange

548b0eb

handle recursive watching ourselves

a718b94

actually drop watcher, use parking_lot::Mutex

091c989

ignore check event dir for ignore, cleanup tests

e313b2c

refactor, put watcher with io::Worker

eca8801

use `RootFilter` to filter recursive watches untested

do not emit create for directory again

cbf87cd

move watcher to io module

b7b11fe

avoid boxing

01b58e0

fix recursive watch

9e0d9da

vemoo added 7 commits January 25, 2019 21:06

simplify and optimize RootFilterby determining if is contained first

d80f7ce

hardcode ".git" and "node_modules" also

30d947d

better test, avoid duplicated events

f47780a

use released notifiy version, add TODO comment

e78e840

review fixes

75fb6b6

extract Roots struct

cd9c59f

use Roots in watcher

1692a11

vemoo force-pushed the vfs-watch branch from 28e44e5 to 1692a11 Compare January 25, 2019 20:18

use entry file_type, improve test

f34b981

matklad mentioned this pull request Jan 26, 2019

Pr 556 #665

Merged

bors bot added a commit that referenced this pull request Jan 26, 2019

Merge #665

c869ee2

665: Pr 556 r=matklad a=matklad Rebased #556 Thanks @vemoo, now I can change branches without reopening VS Code! Co-authored-by: Bernardo <berublan@gmail.com>

matklad closed this Jan 26, 2019

Wilfred mentioned this pull request Jun 2, 2025

Allow individual crates to specify server watching in rust-project.json #19907

Open

VFS watcher impl #556

VFS watcher impl #556

Uh oh!

Conversation

vemoo commented Jan 15, 2019

Uh oh!

Uh oh!

Uh oh!

vemoo Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

vemoo commented Jan 16, 2019

Uh oh!

matklad commented Jan 19, 2019

Uh oh!

vemoo commented Jan 19, 2019

Uh oh!

matklad commented Jan 20, 2019

Uh oh!

vemoo commented Jan 21, 2019

Uh oh!

matklad commented Jan 22, 2019

Uh oh!

Uh oh!

vemoo commented Jan 22, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matklad Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

matklad Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

vemoo Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

vemoo Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

matklad Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vemoo Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

matklad Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

matklad commented Jan 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad commented Jan 30, 2019

Uh oh!

vemoo commented Jan 30, 2019

Uh oh!

vemoo commented Feb 6, 2019

Uh oh!

Uh oh!

matklad Jan 24, 2019 •

edited

Loading

matklad commented Jan 26, 2019 •

edited

Loading