Skip to content

Introduce public interfaces for per-project and per-evaluation disk I/O callbacks #6728

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.

Sign up for GitHub

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

Merged
ladipro merged 23 commits into from
Oct 21, 2021
Merged

Introduce public interfaces for per-project and per-evaluation disk I/O callbacks #6728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
b6fd14c
Introduce IDirectoryCache & IDirectoryCacheFactory
ladipro Aug 3, 2021
7dae629
PR feedback: Pass Project instead of project path
ladipro Aug 4, 2021
9dcb52a
Don't depend on System.IO.Enumeration
ladipro Aug 5, 2021
521d042
Make EngineFileUtilities static
ladipro Aug 5, 2021
e235b15
Implement DirectoryCacheOverFileSystem
ladipro Aug 9, 2021
62737b6
Basic wiring of EvaluationContext-specific FileMatcher
ladipro Aug 9, 2021
10fff0f
Make FileMatcher consume IDirectoryCache instead of IFileSystem
ladipro Aug 10, 2021
200e037
Revert "Make FileMatcher consume IDirectoryCache instead of IFileSystem"
ladipro Aug 17, 2021
1004b54
Introduce DirectoryCacheFileSystemWrapper
ladipro Aug 17, 2021
faeda3e
Avoid string allocations when calling IsMatch
ladipro Aug 17, 2021
1d0a741
Pass IDirectoryCacheFactory to Project ctor (reverts most changes to …
ladipro Aug 18, 2021
65c4942
Fix bugs
ladipro Aug 18, 2021
e0340b7
Use directory cache in all of evaluation
ladipro Aug 19, 2021
77ed3ff
Add unit test
ladipro Aug 19, 2021
90a6bde
Tweaks and bug fixes
ladipro Aug 19, 2021
73d7636
More internal IFileSystem deprecation and tweaks
ladipro Aug 19, 2021
f916592
Add pattern parameter to EnumerateFiles and EnumerateDirectories
ladipro Sep 1, 2021
43f1323
Revert "More internal IFileSystem deprecation and tweaks"
ladipro Oct 15, 2021
89f7cc6
Redo some of the minor tweaks reverted in previous commit
ladipro Oct 15, 2021
e800fa8
Optimize path manipulations (Microsoft.IO.Redist)
ladipro Oct 15, 2021
231a869
Tweaks and comments
ladipro Oct 15, 2021
2348cd8
Remove unused parameter
ladipro Oct 20, 2021
e30d239
Add and update comments
ladipro Oct 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions ref/Microsoft.Build/net/Microsoft.Build.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,7 @@ namespace Microsoft.Build.Definition
public partial class ProjectOptions
{
public ProjectOptions() { }
public Microsoft.Build.FileSystem.IDirectoryCacheFactory DirectoryCacheFactory { get { throw null; } set { } }
public Microsoft.Build.Evaluation.Context.EvaluationContext EvaluationContext { get { throw null; } set { } }
public System.Collections.Generic.IDictionary<string, string> GlobalProperties { get { throw null; } set { } }
public Microsoft.Build.Evaluation.ProjectLoadSettings LoadSettings { get { throw null; } set { } }
Expand Down Expand Up @@ -1504,6 +1505,19 @@ public ProxyTargets(System.Collections.Generic.IReadOnlyDictionary<string, strin
}
namespace Microsoft.Build.FileSystem
{
public delegate bool FindPredicate(ref System.ReadOnlySpan<char> fileName);
public delegate TResult FindTransform<TResult>(ref System.ReadOnlySpan<char> fileName);
public partial interface IDirectoryCache
Copy link
Copy Markdown
Contributor

@cdmihai cdmihai Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to augment MSBuildFileSystemBase with these new methods instead of creating a new interface?. With a new interface, the APIs will get harded to use. Users will have to think whether to implement one or the other, or both, Maybe a new generic MSBuildFileSystemBase that extends MSBuildFileSystemBase.

Copy link
Copy Markdown
Member Author

@ladipro ladipro Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed an update where the interface is now provided via Project and not EvaluationContext so having a separate one should be easier to justify. I would argue that having multiple methods providing the same functionality in MSBuildFileSystemBase would also be confusing for users because it wouldn't be clear which one is used when.

{
bool DirectoryExists(string path);
System.Collections.Generic.IEnumerable<TResult> EnumerateDirectories<TResult>(string path, string pattern, Microsoft.Build.FileSystem.FindPredicate predicate, Microsoft.Build.FileSystem.FindTransform<TResult> transform);
System.Collections.Generic.IEnumerable<TResult> EnumerateFiles<TResult>(string path, string pattern, Microsoft.Build.FileSystem.FindPredicate predicate, Microsoft.Build.FileSystem.FindTransform<TResult> transform);
bool FileExists(string path);
}
public partial interface IDirectoryCacheFactory
{
Microsoft.Build.FileSystem.IDirectoryCache GetDirectoryCacheForEvaluation(int evaluationId);
}
public abstract partial class MSBuildFileSystemBase
{
protected MSBuildFileSystemBase() { }
Expand Down
14 changes: 14 additions & 0 deletions ref/Microsoft.Build/netstandard/Microsoft.Build.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,7 @@ namespace Microsoft.Build.Definition
public partial class ProjectOptions
{
public ProjectOptions() { }
public Microsoft.Build.FileSystem.IDirectoryCacheFactory DirectoryCacheFactory { get { throw null; } set { } }
public Microsoft.Build.Evaluation.Context.EvaluationContext EvaluationContext { get { throw null; } set { } }
public System.Collections.Generic.IDictionary<string, string> GlobalProperties { get { throw null; } set { } }
public Microsoft.Build.Evaluation.ProjectLoadSettings LoadSettings { get { throw null; } set { } }
Expand Down Expand Up @@ -1498,6 +1499,19 @@ public ProxyTargets(System.Collections.Generic.IReadOnlyDictionary<string, strin
}
namespace Microsoft.Build.FileSystem
{
public delegate bool FindPredicate(ref System.ReadOnlySpan<char> fileName);
public delegate TResult FindTransform<TResult>(ref System.ReadOnlySpan<char> fileName);
public partial interface IDirectoryCache
{
bool DirectoryExists(string path);
System.Collections.Generic.IEnumerable<TResult> EnumerateDirectories<TResult>(string path, string pattern, Microsoft.Build.FileSystem.FindPredicate predicate, Microsoft.Build.FileSystem.FindTransform<TResult> transform);
System.Collections.Generic.IEnumerable<TResult> EnumerateFiles<TResult>(string path, string pattern, Microsoft.Build.FileSystem.FindPredicate predicate, Microsoft.Build.FileSystem.FindTransform<TResult> transform);
bool FileExists(string path);
}
public partial interface IDirectoryCacheFactory
{
Microsoft.Build.FileSystem.IDirectoryCache GetDirectoryCacheForEvaluation(int evaluationId);
}
public abstract partial class MSBuildFileSystemBase
{
protected MSBuildFileSystemBase() { }
Expand Down
35 changes: 35 additions & 0 deletions src/Build.UnitTests/Definition/ProjectEvaluationContext_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,41 @@ public void IsolatedContextShouldNotSupportBeingPassedAFileSystem()
Should.Throw<ArgumentException>(() => EvaluationContext.Create(EvaluationContext.SharingPolicy.Isolated, fileSystem));
}

[Fact]
public void EvaluationShouldUseDirectoryCache()
{
var projectFile = _env.CreateFile("1.proj", @"<Project> <ItemGroup Condition=`Exists('1.file')`> <Compile Include='*.cs'/> </ItemGroup> </Project>".Cleanup()).Path;

var projectCollection = _env.CreateProjectCollection().Collection;
var directoryCacheFactory = new Helpers.LoggingDirectoryCacheFactory();

var project = Project.FromFile(
projectFile,
new ProjectOptions
{
ProjectCollection = projectCollection,
DirectoryCacheFactory = directoryCacheFactory,
}
);

directoryCacheFactory.DirectoryCaches.Count.ShouldBe(1);
var directoryCache = directoryCacheFactory.DirectoryCaches[0];

directoryCache.EvaluationId.ShouldBe(project.LastEvaluationId);

directoryCache.ExistenceChecks.OrderBy(kvp => kvp.Key).ShouldBe(
new Dictionary<string, int>
{
{ _env.DefaultTestDirectory.Path, 1},
{ Path.Combine(_env.DefaultTestDirectory.Path, "1.file"), 2 }
}.OrderBy(kvp => kvp.Key));
directoryCache.Enumerations.ShouldBe(
new Dictionary<string, int>
{
{ _env.DefaultTestDirectory.Path, 1 }
});
}

[Theory]
[InlineData(EvaluationContext.SharingPolicy.Shared)]
[InlineData(EvaluationContext.SharingPolicy.Isolated)]
Expand Down
9 changes: 3 additions & 6 deletions src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ internal class ItemGroupIntrinsicTask : IntrinsicTask
/// </summary>
private ProjectItemGroupTaskInstance _taskInstance;

private EngineFileUtilities _engineFileUtilities;

/// <summary>
/// Instantiates an ItemGroup task
/// </summary>
Expand All @@ -41,7 +39,6 @@ public ItemGroupIntrinsicTask(ProjectItemGroupTaskInstance taskInstance, TargetL
: base(loggingContext, projectInstance, logTaskInputs)
{
_taskInstance = taskInstance;
_engineFileUtilities = EngineFileUtilities.Default;
}

/// <summary>
Expand Down Expand Up @@ -431,7 +428,7 @@ ISet<string> removeMetadata
// The expression is not of the form "@(X)". Treat as string

// Pass the non wildcard expanded excludes here to fix https://github.com/Microsoft/msbuild/issues/2621
string[] includeSplitFiles = _engineFileUtilities.GetFileListEscaped(
string[] includeSplitFiles = EngineFileUtilities.GetFileListEscaped(
Project.Directory,
includeSplit,
excludes);
Expand All @@ -455,7 +452,7 @@ ISet<string> removeMetadata

foreach (string excludeSplit in excludes)
{
string[] excludeSplitFiles = _engineFileUtilities.GetFileListUnescaped(Project.Directory, excludeSplit);
string[] excludeSplitFiles = EngineFileUtilities.GetFileListUnescaped(Project.Directory, excludeSplit);

foreach (string excludeSplitFile in excludeSplitFiles)
{
Expand Down Expand Up @@ -540,7 +537,7 @@ Expander<ProjectPropertyInstance, ProjectItemInstance> expander
// Don't unescape wildcards just yet - if there were any escaped, the caller wants to treat them
// as literals. Everything else is safe to unescape at this point, since we're only matching
// against the file system.
string[] fileList = _engineFileUtilities.GetFileListEscaped(Project.Directory, piece);
string[] fileList = EngineFileUtilities.GetFileListEscaped(Project.Directory, piece);

foreach (string file in fileList)
{
Expand Down
Loading