Avoid disk scans for projects by relying on DirectoryTree from CPS #6068
Assignees
Labels
performance
Performance-Scenario-Solution-Open
This issue affects solution open performance.
Priority:1
Work that is critical for the release, but we could probably ship without
size:5
triaged
Milestone
Comments
arunchndr
added
performance
needs-triage
AR-May
added
Performance-Scenario-Solution-Open
panopticoncentral
added
the
Priority:1
Forgind
pushed a commit
that referenced
this issue
May 29, 2021
…ility constraints (#6475) Progress towards #6068 Context MSBuildFileSystemBase was introduced as a replacement of IFileSystem in #5546. It was added as an abstract class so potentially with the same constraints as an interface: Versioning constraints. We can't add a new public member to the class, unless we make it non-abstract and provide a default implementation of new members. Usability constraints. It is not possible to use MSBuildFileSystemBase to intercept calls or override a subset of them because the default MSBuild-provided file system is not exposed. Changes Made To address the constraints above, I am making the class non-abstract and having it by default forward all calls to the default MSBuild file system. This way the caller may choose to override only a subset of methods (e.g. file enumeration and existence/metadata checks) leaving the rest as is (e.g. actual file reads). As a result I was able to delete both IFileSystemAdapter.cs and MSBuildFileSystemAdapter.cs because they're not needed anymore. The difference between IFileSystem methods and public MSBuildFileSystemBase methods was just one method having a different name. I have renamed the one on the internal interface, assuming that this does not cause any troubles with the old NuGet, as tested by TestOldNuget(). This enabled further simplification and reduce the number of the "adapter" call hops. With the changes in this PR, we have: An internal interface IFileSystem, kept around for compat reasons. A public class MSBuildFileSystemBase, which implements IFileSystem with virtual methods. The class is meant to be derived from and its methods overridden. If we find ourselves adding new methods to the contract, it won't break existing MSBuildFileSystemBase because the new calls will just call through to the default implementation. If the change we need to make requires an orchestrated behavior change, we can always add a virtual 'version' prop, overriding which the derived class tells us which version of the contract it supports. Testing Existing unit test coverage. Notes This change is meant to be non-breaking. I'm basically just making an abstract class non-abstract (would have to be done at some point anyways if we were to add a new method). The rest of the changes are internal.
7 tasks
|
Status update: some progress has been done, however there is lots of work left. About 3 weeks of work at CPS which have not started yet and another ~week at MSBuild/CPS side for adapter. |
|
Still on the backburner due to other higher priority work. Expecting to get back to this next week (last week of July). |
|
This has finally bubbled up the priority queue. I'm going to re-estimate the remaining work and make sure the CPS work is also scheduled for the upcoming weeks. This item will miss Preview 3 for sure. |
|
A draft PR is out with the new API which I consider reviewed at this point. There is some work left but it depends on #6075 landing first as it would otherwise introduce temporary workarounds with perf implications. We're also waiting for the CPS work to get planned (AB#1332216). |
|
The MSBuild changes are ready for review. I will wait for the CPS team to validate a private build before merging. Overall, this work is at risk for 17.0 Preview 4 because there is only ~8 days left to merge and integrate everything. |
ladipro
added a commit
that referenced
this issue
Oct 21, 2021
…/O callbacks (#6728) Fixes #6068 ### Context We're looking to use the CPS FS cache for file/directory enumeration and file/directory existence checks done by MSBuild during evaluation. The currently exposed `MSBuildFileSystemBase` base class does not work well for this purpose: 1. Due to its use of `IEnumerable<string>` with full paths. Even if we currently end up constructing full paths internally, the interface should be future-proof. We would like to use something more efficient, preferably close to the modern .NET Core file enumeration API, which we're switching to in another related effort. 2. Due to the scope. `MSBuildFileSystemBase` is applied across all evaluations of all projects sharing the same `EvaluationContext`. CPS wants to know which project is being evaluated and which evaluation (as in evaluation ID) is running. CPS also prefers to set the callbacks for the lifetime of the `Project` object as opposed to having to pass them in `EvaluationContext` for each evaluation. This is because `Project` objects are exposed to other components and anyone can reevaluate them without going through CPS. ### Changes Made 1. Refactoring - Made `EngineFileUtilities` static. This class was wrapping a `FileMatcher` making the code less readable and slightly more complex. A static class is a better fit for the suffix "Utilitites" and `FileMatcher` is explicitly passed to methods that need it. - Made a few evaluation-related classes and methods take `EvaluationContext` rather than `IFileSystem`. The former also has `FileMatcher` on it and it's more efficient to reuse it instead of creating a new one. It is generally more future-proof to be passing around a reference to "context" rather than to only a single part of it. - Made `FileMatcher.IsMatch` take `ReadOnlySpan<char>` instead of string but left the string overload in the code for now. Now that we can use `Microsoft.IO.Redist` several callsites were converted to use the span - the common pattern is to extract the filename from full path and this can be a non-allocating operation on .NET Core and on Framework with `Microsoft.IO.Redist`. 2. Interfaces - Added `IDirectoryCache` with efficient file/directory existence and file/directory enumeration API. The interface is to be implemented by hosts that have their file system caches and want them to be used during evaluation instead of having MSBuild hit disk. - Added `IDirectoryCacheFactory` with a factory method to create `IDirectoryCache` for an evaluation ID. The host can choose to return null to make MSBuild fall back to the existing behavior of hitting disk. `IDirectoryCacheFactory` is passed as a field on `ProjectOptions`. If the host specifies this interface and also an evaluation context with a `MSBuildFileSystemBase`-derived class, this new interface will be used for file/directory existence checks and file/directory enumeration while the rest of the operations (are there any such operations used during evaluation?) would go to `MSBuildFileSystemBase`. ### Testing Existing and new unit tests, experimental insertion, validation by the CPS team.
|
For completeness: MSBuild changes made it into 17.1 P1 but the feature is planned to be enabled in 17.1 P2, more details in this internal work item. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Child of #6034.
Advantage is avoid slow disk scan and can avoid temp files from being picked up into the glob and thereby triggering an evaluation of the same (This also causes unwanted UI tree changes).
CPS scans the project cone directory and maintain an in-memory copy of it. If in-VS msbuild evaluation can use the virtual directory state to resolve globbing, it will cutdown time to hit disk, especially during project changes. It will also eliminate some bugs, when globbing picks up temporary file, because the project system and msbuild evaluation doesn’t have a good way to align the disk state when evaluation happens.
[ladipro] Notes from initial investigation:
MSBuildFileSystemBasetoEvaluationContext.Create.MSBuildFileSystemBaseso the caller has to provide all functionality by implementing all abstract methods.MSBuildFileSystemBasewas created with future needs in mind and some of the functions are not currently used.Tasks:
MSBuildFileSystemBaseandIProjectTreein CPS (size:3).The text was updated successfully, but these errors were encountered: