Watch directories for source files and some metadata references#42180
Watch directories for source files and some metadata references#421802 commits merged intodotnet:masterfrom
Conversation
|
I've started a signed build so we can get an RPS run for this change. |
a28a630 to
ccbaa92
Compare
|
Running a RPS run internally, this did hit one regression. Since we're watching the directory now, during Rebuild we're getting events that we weren't previously. It seems that there's a fair amount of allocation overhead from that in the file watching service on the order of 140 MB or so, which is almost enough to trip the regression flag. Will be sending an email to the owners of the service to see if there's some improvements we can make here. |
ccbaa92 to
c8cff54
Compare
c8cff54 to
c7c304d
Compare
|
Switching this PR back to draft because right now it's using reflection as the VS platform team fixes a NuGet packaging issue with their packages. A new RPS run has been kicked off to investigate. |
c7c304d to
30901c8
Compare
Many code changes since last review.
There was a problem hiding this comment.
This will be fixed up in master-vs-deps; unfortunately we don't have 16.7 images yet (those come tomorrow) so there's no way it'd work until then.
There was a problem hiding this comment.
we don't have 16.7 images yet (those come tomorrow)
I'm willing to wait until tomorrow for you to not use reflection.
There was a problem hiding this comment.
I have low confidence it'll work on the images we're getting since those were Preview 1 images and I think the API changes we needed went into Preview 2. Putting this in master and then removing the reflection in master-vs-deps is also helpful because it limits the code skew between the two branches.
There was a problem hiding this comment.
open tracking issue to get us off reflectio nhere?
There was a problem hiding this comment.
I'm just going to make the PR later today
src/VisualStudio/Core/Def/Implementation/ProjectSystem/FileChangeWatcher.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/ProjectSystem/FileChangeWatcher.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
so this work is async. i don't really understand the lifetimes of this async work, esp. wrt to unadvising.
There was a problem hiding this comment.
We put all the actual calls to the service on a single queue, since the underlying VS service is more or less single threaded. Thus each operation here is added to a queue to be processed at a later time. The later unadvising still adds the operations to that queue.
What about the unadvising is confusing?
There was a problem hiding this comment.
- boy is that a terrifying comment. i'm going to presuem there's a big caveat of "because if someone else was accessing this concurrently, they'd be violating the disposal contract, and then all bets are off.
- would IAsyncDisposable be preferable here?
There was a problem hiding this comment.
- Yes, this is the standard "after dispose, it doesn't matter" case.
- I don't think so, since we wouldn't want it being awaited -- if I close a project there's no reason to block that action on our ability to stop watching files. That can happen later, or never in the case of an outright close of VS.
There was a problem hiding this comment.
ok. worth doc'ing that.
There was a problem hiding this comment.
Comments updated.
src/VisualStudio/Core/Def/Implementation/ProjectSystem/FileChangeWatcher.cs
Outdated
Show resolved
Hide resolved
...ementation/ProjectSystem/MetadataReferences/FileWatchedPortableExecutableReferenceFactory.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
is it gauranteed this folder exists? if it doesn't do we throw? or just silently go on?
There was a problem hiding this comment.
The only way it wouldn't exist is you don't have any .NET Framework SDK at all on the machine. If it didn't exist MiscellaneousFilesWorkspace would also be broken, because there'd be no references to pick up at all either for that.
...ementation/ProjectSystem/MetadataReferences/FileWatchedPortableExecutableReferenceFactory.cs
Outdated
Show resolved
Hide resolved
|
I don't quite get the filtering. How it works or why we need it. Can you clarify? Done with pass. |
|
In our first attempt to do a single watch for all source files in the project, it improved memory usage and solution load/unload; the it hurt build time, and that was because by default the bin and obj folders are also under the project folder. When you built, all of the writing to temporary files there and the .dll or .exe was raising a bunch of extra change events. The VS layer has a bunch of overhead that once it knows somebody needs to be notified, it then creates a bunch of tasks and adds them to a queue for notification and certain synchronization. By knowing we don't even want those, it gave us an easy way to reduce that overhead. |
There was a problem hiding this comment.
Keep as an expression-bodied-method?
There was a problem hiding this comment.
Meh. I can't say I care for them much beyond trivial accessors anyways. :-)
|
Do we have issues with things like editorconfig files or additional-files? |
|
@CyrusNajmabadi We shouldn't. These directory watches are purely as an optimization; anything not matched in the directory watch (either it's outside the directory, or the extension doesn't match) still gets an explicit file watch added so we'll still pick it up. |
30901c8 to
504c226
Compare
|
Got it. do you think that's documented well enough for the future? |
|
@CyrusNajmabadi I clarified a bit more in the PR message but I think we're OK. |
This is a performance improvement to avoid creating so many file watchers in Visual Studio. The Visual Studio file watching service consolidates file watchers to a single directory, but still has to manage the list of all of the file names we are trying to watch so it can appropriately filter, and the work to create all of that tracking is entirely unnecessary since in practice we're just going to watch all the files of a particular extension under a directory. This is an optimization in that the FileChangeWatcher and does not impact correctness. When the FileChangeWatcher is asked to watch a file, it checks to see if the the file is under the directory and matches any extension filter; if not, it'll still create a regular file watch under the covers. This is opaque to the users, since they're getting file watches and will see change notifications either way. It's just a performance hint to avoid unnecessary work. This required updating our mock implementation of the Visual Studio file change service to support requests for directory watching and for handling the filtering of them. Fixes dotnet#13679 Fixes dotnet#42095
504c226 to
0bb2853
Compare
cb21eda to
f9756dd
Compare
This is a performance improvement to avoid creating so many file watchers in Visual Studio. The Visual Studio file watching service consolidates file watchers to a single directory, but still has to manage the list of all of the file names we are trying to watch so it can appropriately filter, and the work to create all of that tracking is entirely unnecessary since in practice we're just going to watch all the files of a particular extension under a directory.
This is an optimization in that the FileChangeWatcher and does not impact correctness. When the FileChangeWatcher is asked to watch a file, it checks to see if the the file is under the directory and matches any extension filter; if not, it'll still create a regular file watch under the covers. This is opaque to the users, since they're getting file watches and will see change notifications either way. It's just a performance hint to avoid unnecessary work.
This required updating our mock implementation of the Visual Studio file change service to support requests for directory watching and for handling the filtering of them.
Fixes #13679
Fixes #42095