ShadowCopyAnalyzerPathResolver: Use cache to amortize cost of AV scans - #84765
ShadowCopyAnalyzerPathResolver: Use cache to amortize cost of AV scans#84765RikkiGibson wants to merge 19 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR changes the analyzer shadow-copy mechanism to optionally hard-link shadow-copied analyzer assemblies through a shared cache directory (primarily for Windows), aiming to reduce repeated overhead (e.g., AV rescans) across runs. It also introduces new file utility helpers to create hard links and to query hard-link counts for cache pruning.
Changes:
- Add
FileUtilities.TryCreateHardLinkandFileUtilities.CountHardLinks(Win32 P/Invokes) to support hard-linking and pruning logic. - Extend
ShadowCopyAnalyzerPathResolverwith aCacheDirectory, hard-link-from-cache / hard-link-to-cache behavior, and a best-effort cache pruning step during cleanup. - Add hashing helpers (
HashToHex,GetCacheKey) to derive stable cache filenames.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Compilers/Core/Portable/FileSystem/FileUtilities.cs | Adds Win32 hard-link creation + hard-link-count querying helpers used by the shadow-copy cache. |
| src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs | Adds cache directory support, cache pruning, and hard-linking behavior when shadow-copying analyzer assemblies. |
Suppressed comments (3)
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:333
copyFiletakesoriginalPathas a parameter, but this call uses the outeroriginalFilePathinstead. They are currently the same, but this makes the local function easier to misuse/refactor incorrectly later and is inconsistent with the parameter naming.
if (File.Exists(originalPath))
{
linkFromCacheOrFallbackToCopy(originalFilePath, shadowCopyPath);
ClearReadOnlyFlagOnFile(new FileInfo(shadowCopyPath));
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:349
GetCacheKeycallsAssemblyUtilities.ReadMvid, which throws for non-assemblies. Existing unit tests for this resolver shadow-copy plain text files (e.g. writing "test" toanalyzer.dll), so this change will start throwingBadImageFormatExceptioninstead of copying. Also, even if this resolver is ever used on non-Windows, the cache key computation adds extra I/O despite always falling back toFile.Copy. Consider: (1) short-circuiting the cache logic entirely when not on Windows, and (2) treating failures to compute a cache key as a signal to fall back to a normal copy to preserve prior behavior.
void linkFromCacheOrFallbackToCopy(string originalPath, string shadowCopyPath)
{
var cachePath = Path.Combine(CacheDirectory, GetCacheKey(originalPath));
if (File.Exists(cachePath))
{
// File is already present in cache. First try to hard-link from cache to the shadow copy path. Failing that just copy from the original path.
if (!PlatformInformation.IsWindows || !haveMatchingMvidAndSize(originalPath, cachePath) || !FileUtilities.TryCreateHardLink(cachePath, shadowCopyPath))
File.Copy(originalPath, shadowCopyPath);
}
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:219
Directory.EnumerateFiles(CacheDirectory)will throw when the cache directory hasn't been created yet. This gets swallowed by the outercatch, but it means we rely on exceptions for the common case (no cache) and silently skip any pruning work. Add an explicitDirectory.Existscheck inside thelockTakenblock before enumerating.
// Permit up to 200 unlinked files (not hard-linked to a specific shadow loader directory).
// Delete the oldest files which exceed this limit.
const int maxUnlinkedCount = 200;
var filesToEvict = Directory.EnumerateFiles(CacheDirectory)
.Where(file =>
| if (subDirectory == CacheDirectory) | ||
| continue; |
There was a problem hiding this comment.
This specific comparison was deleted, unsure what the other ones were, we'll see if it brings it back up or if we can spot it. I don't think I introduced any new use of == to compare paths.
| public static bool TryCreateHardLink(string path, string pathToTarget) | ||
| { | ||
| return CreateHardLink(pathToTarget, path, IntPtr.Zero); | ||
|
|
There was a problem hiding this comment.
It was intentional to use same parameter order and names as https://learn.microsoft.com/en-us/dotnet/api/system.io.file.createhardlink?view=net-11.0.
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:132
- Skipping the cache directory via
subDirectory == CacheDirectoryis brittle on Windows (path casing/normalization differences can make this false even for the same directory). If this fails, the cleanup loop may delete the cache directory, negating the optimization. Use the existing path comparer (e.g.,AnalyzerAssemblyLoader.OriginalPathComparer) or compare just the directory name with an ignore-case comparison.
if (subDirectory == CacheDirectory)
continue;
src/Compilers/Core/Portable/FileSystem/FileUtilities.cs:466
TryCreateHardLink's implementation inverts the parameter semantics ofFile.CreateHardLink(path, pathToTarget)(wherepathis the new link location andpathToTargetis the existing file). With the currentCreateHardLink(pathToTarget, path, ...)call, any future caller using it like the BCL API will create the link in the wrong place. Consider aligning this helper with the BCL semantics and updating callers accordingly (and keeping the XML doc link accurate).
public static bool TryCreateHardLink(string path, string pathToTarget)
{
return CreateHardLink(pathToTarget, path, IntPtr.Zero);
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:230
pruneCacheIfNeededenumeratesCacheDirectoryunconditionally and relies on the outer catch to swallowDirectoryNotFoundExceptionwhen the cache directory hasn't been created. This will commonly incur avoidable exceptions. Also, this is compiler product code insrc/Compilers/—prefer avoiding the LINQ pipeline here to reduce allocations and make cost clearer.
// Permit up to 200 unlinked files (not hard-linked to a specific shadow loader directory).
// Delete the oldest files which exceed this limit.
const int maxUnlinkedCount = 200;
var filesToEvict = Directory.EnumerateFiles(CacheDirectory)
.Where(file =>
{
Debug.Assert(PlatformInformation.IsWindows);
return FileUtilities.CountHardLinks(file) == 1;
})
.OrderByDescending(File.GetLastWriteTimeUtc)
.Skip(maxUnlinkedCount);
foreach (var file in filesToEvict)
{
File.Delete(file);
}
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:349
- The cache key computation (
GetCacheKey) reads the assembly MVID even on non-Windows platforms (where the cache is never populated/pruned). This adds extra I/O/metadata reads on Linux/macOS for every shadow-copied analyzer without any benefit. Consider an early!PlatformInformation.IsWindowsfast-path that just copies the file.
void linkFromCacheOrFallbackToCopy(string originalPath, string shadowCopyPath)
{
var cachePath = Path.Combine(CacheDirectory, GetCacheKey(originalPath));
if (File.Exists(cachePath))
{
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:341
- This PR introduces new behaviors (cache population, hard-linking vs copying, cache pruning) but existing unit tests for
ShadowCopyAnalyzerPathResolveronly validate basic copy/grouping behavior. Adding regression tests would help ensure: (1) the non-Windows fast-path still just copies, (2) hard-link failures reliably fall back to copying, and (3) cache cleanup does not delete active/in-use cached files.
// Optimization for antivirus scanning on Windows:
// - Shadow copied files are hard-linked to/from a cache directory if possible.
// - We continue to use per-session 'ShadowDirectory' for ease of implementing correct loading semantics and cleanup.
// - Hard linking a file from the cache instead of copying it is empirically observed to reduce time spent running AV scans when loading assemblies.
void linkFromCacheOrFallbackToCopy(string originalPath, string shadowCopyPath)
There was a problem hiding this comment.
Consider starting a validation VS insertion for this change.
| // - Hard linking a file from the cache instead of copying it is empirically observed to reduce time spent running AV scans when loading assemblies. | ||
| void linkFromCacheOrFallbackToCopy(string originalPath, string shadowCopyPath) | ||
| { | ||
| var cachePath = Path.Combine(CacheDirectory, GetCacheKey(originalPath)); |
There was a problem hiding this comment.
It feels like on non-Windows, we don't even need to compute this and call File.Exists for it.
|
|
||
| return hashToHex(hash); | ||
|
|
||
| // See also 'PrivateImplementationDetails.HashToHex' |
There was a problem hiding this comment.
Perhaps we could extract this into some shared utility?
| { | ||
| // Key format: (original filename) + (file path hash) + (mvid) | ||
| var hexHash = HashToHex(originalPath); | ||
| var mvid = AssemblyUtilities.ReadMvid(originalPath); |
There was a problem hiding this comment.
It seems this might throw for corrupted files where we previously didn't throw (and it also seems some CI tests caught this).
|
|
||
| foreach (var subDirectory in subDirectories) | ||
| { | ||
| if (subDirectory == CacheDirectory) |
There was a problem hiding this comment.
Could we just place the cache directory elsewhere? So that also older roslyn versions wouldn't delete it?
There was a problem hiding this comment.
I think we should, it's just painful to have multiple roslyn versions potentially operating on the same directories here. Can/should we introduce the Roslyn version itself as a path component somewhere to avoid that?
To solve for now I added a second parameter. This affects an OmniSharp EA but I was advised that making changes to that is fine, O# can react when/if it updates its Roslyn version.
| { | ||
| // File is already present in cache. First try to hard-link from cache to the shadow copy path. Failing that just copy from the original path. | ||
| if (!PlatformInformation.IsWindows || !haveMatchingMvidAndSize(originalPath, cachePath) || !FileUtilities.TryCreateHardLink(cachePath, shadowCopyPath)) | ||
| File.Copy(originalPath, shadowCopyPath); |
There was a problem hiding this comment.
If we fall back to copying should we delete the cache entry to avoid needing to check the mvid etc every time?
| if (!PlatformInformation.IsWindows) | ||
| return; |
There was a problem hiding this comment.
Why are you checking for Windows here? This code should run equally well on Windows or Linux. The decision point is usually whether to run this at all there.
There was a problem hiding this comment.
I didn't think there was a need/benefit to use the cache on Linux. We could strictly make the code work on Linux also, but, it would require stubbing out more native methods. I guess it is the case that we usually avoid running this code on Linux by simply not using this path resolver by default there.
There was a problem hiding this comment.
There also isn't a benefit to using shadow copy at all on Linux. That's why this type isn't used there. Basically this entire type is a windows specific artifact, nowhere else do we do sub-feature checknig for windows.
There was a problem hiding this comment.
I am leaning toward resolving by checking PlatformInformation.IsWindows in constructor, and throwing if that is not met. Then Debug.Assert()ing if we need to in order to make the platform analyzer happy.
Because if we want all of this code to "work" on linux, it's signing up for additional work to make things like cache pruning work sensibly on linux, e.g. by writing the native call to count the links.
…zerPathResolver.cs Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/Compilers/Core/Portable/FileSystem/FileUtilities.cs:463
TryCreateHardLink’s XML doc points toSystem.IO.File.CreateHardLink, but this helper’s parameter order/semantics are the inverse (the current implementation treats the first parameter as the existing file and the second as the new link path). This mismatch makes it easy for future call sites to accidentally reverse the arguments.
/// <seealso href="https://learn.microsoft.com/en-us/dotnet/api/system.io.file.createhardlink?view=net-11.0" />
#if NET
[SupportedOSPlatform("windows")]
#endif
public static bool TryCreateHardLink(string path, string pathToTarget)
src/Features/ExternalAccess/OmniSharp/Analyzers/OmnisharpAnalyzerLoaderFactory.cs:15
- This change removes the optional
baseDirectoryparameter, butInternalAPI.Unshipped.txtstill listsCreateShadowCopyAnalyzerAssemblyLoader(string? baseDirectory = null). Restoring the original signature here avoids an internal-API mismatch (and preserves flexibility for callers) while still allowing a cache directory to be introduced.
public static IAnalyzerAssemblyLoader CreateShadowCopyAnalyzerAssemblyLoader()
{
var baseDirectory = Path.Combine(Path.GetTempPath(), "CodeAnalysis", "OmnisharpAnalyzerShadowCopies");
var cacheDirectory = Path.Combine(Path.GetTempPath(), "CodeAnalysis", "OmnisharpAnalyzerShadowCopies-cache");
return AnalyzerAssemblyLoader.CreateNonLockingLoader(baseDirectory, cacheDirectory);
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:224
pruneCacheIfNeededenumeratesCacheDirectorywithout checking whether it exists. On first run (or when caching was never used) this can throwDirectoryNotFoundException, which gets swallowed but still incurs repeated first-chance exceptions and prevents pruning from running.
// Permit up to 200 unlinked files (not hard-linked to a specific shadow loader directory).
// Delete the oldest files which exceed this limit.
const int maxUnlinkedCount = 200;
var filesToEvict = Directory.EnumerateFiles(CacheDirectory)
.Where(static file =>
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:337
- Inside
copyFile, the call tolinkFromCacheOrFallbackToCopyuses the outeroriginalFilePathinstead of theoriginalPathparameter. This currently works because the caller passes the same value, but it makes the local function easy to misuse if it’s refactored or reused later.
if (File.Exists(originalPath))
{
linkFromCacheOrFallbackToCopy(originalFilePath, shadowCopyPath);
ClearReadOnlyFlagOnFile(new FileInfo(shadowCopyPath));
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:346
- This PR introduces new caching + hard-linking behavior in the shadow-copy path, but the existing
ShadowCopyAnalyzerPathResolverTestswere only updated for the new constructor signature. It would be good to add focused tests that (on Windows) validate: (1) cache population creates an additional hard link, (2) a subsequent resolve uses a hard link instead of a copy when possible, and (3) behavior still succeeds when hard-link creation fails (e.g., different volume / unsupported FS).
// Optimization for antivirus scanning on Windows:
// - Shadow copied files are hard-linked to/from a cache directory if possible.
// - We continue to use per-session 'ShadowDirectory' for ease of implementing correct loading semantics and cleanup.
// - Hard linking a file from the cache instead of copying it is empirically observed to reduce time spent running AV scans when loading assemblies.
void linkFromCacheOrFallbackToCopy(string originalPath, string shadowCopyPath)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/Compilers/Core/Portable/FileSystem/FileUtilities.cs:466
TryCreateHardLink's signature and<seealso>point atFile.CreateHardLink(path, pathToTarget), but the implementation calls the Win32 API with the arguments reversed (CreateHardLink(pathToTarget, path, ...)). With the new caching call sites added, this mismatch is likely to cause accidental misuse (especially with named arguments) and makes it unclear which order callers should use. Consider aligning the implementation to the documented .NET order, and updating the new call sites accordingly (or, alternatively, update the XML/docs/parameter names to reflect the actual order).
/// <summary>Create a hard link to a file.</summary>
/// <seealso href="https://learn.microsoft.com/en-us/dotnet/api/system.io.file.createhardlink?view=net-11.0" />
#if NET
[SupportedOSPlatform("windows")]
#endif
public static bool TryCreateHardLink(string path, string pathToTarget)
{
return CreateHardLink(pathToTarget, path, IntPtr.Zero);
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs:409
- The
CreateNonLockingLoaderXML doc comment documentswindowsShadowPath, but not the newly addedwindowsCachePathparameter. Adding a<param>entry here will help callers understand the cache requirements (same volume, best-effort optimization).
/// <summary>
/// Return an <see cref="IAnalyzerAssemblyLoader"/> which does not lock assemblies on disk that is
/// most appropriate for the current platform.
/// </summary>
/// <param name="windowsShadowPath">A shadow copy path will be created on Windows and this value
/// will be the base directory where shadow copy assemblies are stored. </param>
internal static IAnalyzerAssemblyLoaderInternal CreateNonLockingLoader(
string windowsShadowPath,
string windowsCachePath,
ImmutableArray<IAnalyzerPathResolver> pathResolvers = default,
src/Features/ExternalAccess/OmniSharp/Analyzers/OmnisharpAnalyzerLoaderFactory.cs:16
CreateShadowCopyAnalyzerAssemblyLoaderremoved the optionalbaseDirectoryparameter, but this API is tracked inInternalAPI.Unshipped.txtand may be consumed externally. Removing the parameter is a breaking change and will also desync the internal API baseline. Consider keeping the original signature and deriving the cache directory from the chosen base directory.
public static IAnalyzerAssemblyLoader CreateShadowCopyAnalyzerAssemblyLoader()
{
var baseDirectory = Path.Combine(Path.GetTempPath(), "CodeAnalysis", "OmnisharpAnalyzerShadowCopies");
var cacheDirectory = Path.Combine(Path.GetTempPath(), "CodeAnalysis", "OmnisharpAnalyzerShadowCopies-cache");
return AnalyzerAssemblyLoader.CreateNonLockingLoader(baseDirectory, cacheDirectory);
}
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:235
pruneCacheIfNeededuses multiple LINQ operators (Where/OrderByDescending/Skip) overDirectory.EnumerateFiles, which will allocate and sort the entire file list before deletion. Since this is compiler product code and may run on startup, consider using an explicit loop + sort to reduce allocations and make the cost more obvious.
// Permit up to 200 unlinked files (not hard-linked to a specific shadow loader directory).
// Delete the oldest files which exceed this limit.
const int maxUnlinkedCount = 200;
var filesToEvict = Directory.EnumerateFiles(CacheDirectory)
.Where(static file =>
{
Debug.Assert(PlatformInformation.IsWindows);
return FileUtilities.CountHardLinks(file) == 1;
})
.OrderByDescending(File.GetLastWriteTimeUtc)
.Skip(maxUnlinkedCount);
foreach (var file in filesToEvict)
{
File.Delete(file);
}
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:339
copyFiletakesoriginalPath, but the call tolinkFromCacheOrFallbackToCopyuses the outeroriginalFilePathinstead. They are currently the same value, but using the parameter makes the helper less error-prone if the surrounding code changes.
// The shadow copy should only copy files that exist. For files that don't exist, this best
// emulates not having the shadow copy layer
if (File.Exists(originalPath))
{
linkFromCacheOrFallbackToCopy(originalFilePath, shadowCopyPath);
ClearReadOnlyFlagOnFile(new FileInfo(shadowCopyPath));
}
src/Compilers/Core/CodeAnalysisTest/ShadowCopyAnalyzerPathResolverTests.cs:26
- The resolver now takes an explicit
cacheDirectoryand contains new cache/hard-link behavior (including the fallback path when hard links fail). The existing tests validate basic shadow copying but don't appear to cover the cache behavior or a "hard links unsupported" scenario. Consider adding focused tests that (1) verify a second shadow copy reuses a cached entry when possible, and (2) verify functionality when hard-link creation always fails.
public ShadowCopyAnalyzerPathResolverTests()
{
TempRoot = new TempRoot();
PathResolver = new ShadowCopyAnalyzerPathResolver(TempRoot.CreateDirectory().Path, TempRoot.CreateDirectory().Path);
}
…nto shadow-hard-link # Conflicts: # src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs
|
/pr-val ee53d7f |
|
View PR Validation Run triggered by @RikkiGibson Parameters
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (8)
src/Workspaces/Core/Portable/Workspace/Host/Metadata/IAnalyzerAssemblyLoaderProvider.cs:74
- Same indentation issue as above in the non-NET branch: the cache path argument should be aligned with the other arguments for readability/consistency.
public IAnalyzerAssemblyLoaderInternal CreateNewShadowCopyLoader()
=> this.WrapLoader(AnalyzerAssemblyLoader.CreateNonLockingLoader(
Path.Combine(Path.GetTempPath(), nameof(Roslyn), "AnalyzerAssemblyLoader"),
Path.Combine(Path.GetTempPath(), nameof(Roslyn), "AnalyzerAssemblyLoader-cache"),
pathResolvers: default));
src/Compilers/Core/Portable/FileSystem/FileUtilities.cs:13
- The newly added Windows-specific
usingdirectives are unused in this file, which adds unnecessary dependencies and may introduce warnings-as-errors in some builds. Remove the unused imports.
using System.Runtime.InteropServices;
using System.Runtime.Versioning;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.Win32.SafeHandles;
src/Workspaces/Core/Portable/Workspace/Host/Metadata/IAnalyzerAssemblyLoaderProvider.cs:58
- The second argument line is mis-indented, which makes this call harder to read and inconsistent with the surrounding formatting. Align the cache path argument with the other arguments.
This issue also appears on line 70 of the same file.
=> this.WrapLoader(AnalyzerAssemblyLoader.CreateNonLockingLoader(
Path.Combine(Path.GetTempPath(), nameof(Roslyn), "AnalyzerAssemblyLoader"),
Path.Combine(Path.GetTempPath(), nameof(Roslyn), "AnalyzerAssemblyLoader-cache"),
_assemblyPathResolvers,
_assemblyResolvers));
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs:408
windowsCachePathwas added to the NET build signature but the XML doc comment wasn't updated. Please document this parameter (as is done in the non-NET build) so callers understand the same-volume requirement and the effect on caching.
/// <param name="windowsShadowPath">A shadow copy path will be created on Windows and this value
/// will be the base directory where shadow copy assemblies are stored. </param>
internal static IAnalyzerAssemblyLoaderInternal CreateNonLockingLoader(
string windowsShadowPath,
string windowsCachePath,
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:345
- Inside
copyFile, the existence check uses theoriginalPathparameter, but the subsequent cache/copy operation uses the outeroriginalFilePathvariable. They happen to be the same today, but using the parameter avoids accidental divergence if this helper is reused/refactored.
if (File.Exists(originalPath))
{
linkFromCacheOrFallbackToCopy(originalFilePath, shadowCopyPath);
ClearReadOnlyFlagOnFile(new FileInfo(shadowCopyPath));
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:490
TryCreateHardLink's parameter names imply the same semantics/order asFile.CreateHardLink(path, pathToTarget)(wherepathis the new link), but the implementation treats the first argument as the existing file and the second as the new link. Renaming parameters to match actual semantics will prevent accidental misuse at call sites.
private static bool TryCreateHardLink(string path, string pathToTarget)
{
return CreateHardLink(pathToTarget, path, IntPtr.Zero);
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:450
- The cache key includes the original file name plus additional metadata, which can easily exceed Windows' per-path-segment limits (e.g. long analyzer DLL names). That would turn caching into a potential functional failure (path too long / invalid file name). Consider using a fixed-length key (hash + mvid + length + extension) instead of embedding the original file name.
// Key format: (original filename) + (file path hash) + (mvid) + (original file length)
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:354
- New cache/hard-link behavior is introduced here (cache hit, cache miss, hard-link success, hard-link failure fallback, and cache pruning), but there are no dedicated unit tests exercising these branches. Adding targeted tests (Windows-only where needed) would help prevent regressions in the caching/fallback semantics.
// Optimization for antivirus scanning on Windows:
// - Shadow copied files are hard-linked to/from a cache directory if possible.
// - We continue to use per-session 'ShadowDirectory' for ease of implementing correct loading semantics and cleanup.
// - Hard linking a file from the cache instead of copying it is empirically observed to reduce time spent running AV scans when loading assemblies.
void linkFromCacheOrFallbackToCopy(string originalPath, string shadowCopyPath)
{
| string windowsShadowPath, | ||
| string windowsCachePath, |
There was a problem hiding this comment.
Why not have one base path and then build cache and shadow off of that?
There was a problem hiding this comment.
See also #84765 (comment)
My concern about changing the layout of the windowsShadowPath was, if we don't also change its location at the same time, then we could have old and new versions of this type both messing with the same windowsShadowPath at the same time. Potentially old versions attempting to clean up the wrong things, etc.
There was a problem hiding this comment.
Even if that's the aces, can't we have one path and then just change the default we use for the shadwo path to avoid conflicts?
| if (!PlatformInformation.IsWindows) | ||
| return; |
There was a problem hiding this comment.
There also isn't a benefit to using shadow copy at all on Linux. That's why this type isn't used there. Basically this entire type is a windows specific artifact, nowhere else do we do sub-feature checknig for windows.
|
|
||
| if (lockTaken) | ||
| { | ||
| // Permit up to 200 unlinked files (not hard-linked to a specific shadow loader directory). |
There was a problem hiding this comment.
I arbitrarily decided a few hundred is a decent amount.
I'll look at whether we can just expose stats here, or leverage existing stats to make a better decision (how many distinct analyzer dlls did we resolve through this). Then we could just try building various things and see what numbers we get. How many do we load, how much do we end up pruning, and how much disk utilization, if we build large solution A then B then C all on the same machine.
There was a problem hiding this comment.
Even if it's arbitrary let's document why. That way future devs know what they're working with if they get evidence to change this.
| Debug.Assert(PlatformInformation.IsWindows); | ||
| return (file, fileInformationOpt: TryGetWindowsFileInformation(file)); | ||
| }) | ||
| .Where(static pair => pair.fileInformationOpt is { NumberOfLinks: 1 }) |
There was a problem hiding this comment.
What happens if another instance of this type is in another process racing to create links to these files in parallel? Even if this is safe discuss this in comments: both for future devs and future AI to take into consideration when making changes.
| } | ||
|
|
||
| static void copyFile(string originalPath, string shadowCopyPath) | ||
| void copyFile(string originalPath, string shadowCopyPath) |
There was a problem hiding this comment.
I would keep this static and explicitly pass state because it's super easy to capture the wrong value here create good looking but incorrect code.
| if (!PlatformInformation.IsWindows) | ||
| { | ||
| File.Copy(originalPath, shadowCopyPath); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This platform checking feels wrong. The caller passed a cache path and you're silently ignoring it here. Feel like the construction of the type should be specifying the usage.
|
|
||
| private static string? TryGetCacheKey(string originalPath) | ||
| { | ||
| // Key format: (original filename) + (file path hash) + (mvid) + (original file length) |
There was a problem hiding this comment.
Comment doesn't match implementation which includes extension.
| } | ||
| } | ||
|
|
||
| private static string HashToHex(string value) |
There was a problem hiding this comment.
| private static string HashToHex(string value) | |
| private static string HashToHex(ReadOnlySpan<char> value) |
|
Test insertion is passing CloudBuild+RPS+Speedometer with neither regressions or improvements. https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/766898 |
|
I added some benchmark numbers to the PR description which give some evidence that hard linking+loading is faster than copy+loading. I also had my LLM write some additional benchmarks offline for following scenarios. These felt more "throwaway/exploratory" to me, so I didn't push them.
I don't find it super clear why the dll events are entirely missing from the "with cache" case in the defender performance report. Or, why the events are listed as 'OnClose'. It doesn't seem like the scan is occurring as part of the file copy, but, it's hard to be sure. What seems clear from what we can measure though is that hard linking is helping here. |
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/Compilers/Core/Portable/FileSystem/FileUtilities.cs:13
- The added interop/platform-related
usingdirectives appear to be unused in this file (no references toMarshal,SupportedOSPlatform, orSafeFileHandle). Unused usings commonly produce build warnings and can be treated as errors in this repo; please remove them.
using System.Runtime.InteropServices;
using System.Runtime.Versioning;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.Win32.SafeHandles;
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:462
TryCreateHardLink's parameter names imply the same semantics asFile.CreateHardLink(path, pathToTarget), but the P/Invoke call currently swaps them (CreateHardLink(pathToTarget, path, ...)). This makes the helper very easy to misuse and contradicts the doc link/comment. Rename the parameters to reflect the actual semantics (existing file path vs. new link path) and pass them to the Win32 API in that order.
/// <summary>Create a hard link to a file.</summary>
/// <seealso href="https://learn.microsoft.com/en-us/dotnet/api/system.io.file.createhardlink?view=net-11.0" />
private static bool TryCreateHardLink(string path, string pathToTarget)
{
return CreateHardLink(pathToTarget, path, IntPtr.Zero);
// https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createhardlinkw
[DllImport("Kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)]
static extern bool CreateHardLink(string lpFileName, string lpExistingFileName, IntPtr lpSecurityAttributes);
}
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:234
- The FILETIME-to-Int64 conversion used for cache eviction ordering sign-extends
dwHighDateTimebecause it’s anint. Treat the high DWORD as unsigned to avoid incorrect ordering once the high bit is set.
var creationTime = pair.fileInformationOpt!.Value.CreationTime;
return (long)creationTime.dwHighDateTime << 32 | (uint)creationTime.dwLowDateTime;
})
src/Tools/Benchmarks/AssemblyFileOperationBenchmarks.cs:32
GlobalSetupthrows on non-Windows. This benchmark project is validated in BenchmarkDotNet "Dry" mode (seeeng/validate-benchmarks.ps1), and this exception will fail validation on non-Windows machines even though only the hard-link scenarios are Windows-specific. Consider making the hard-link benchmarks gracefully no-op or fall back to copy on non-Windows so the suite remains runnable cross-platform.
if (!OperatingSystem.IsWindows())
{
throw new PlatformNotSupportedException("Hard-link creation is benchmarked through the Windows API.");
}
src/Compilers/Core/CodeAnalysisTest/ShadowCopyAnalyzerPathResolverTests.cs:10
System.Runtime.InteropServicesis imported but not used in this test file. Please remove it to avoid unused-using warnings.
using System.Linq;
using System.Runtime.InteropServices;
using System.Runtime.Versioning;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:457
TryCreateHardLink(string path, string pathToTarget)reads likeFile.CreateHardLink(path, pathToTarget), but the implementation and call sites treat the first parameter as the existing file and the second as the new link path. Renaming the parameters (and updating the summary) would make the contract explicit and reduce the chance of accidental misuse.
/// <summary>Create a hard link to a file.</summary>
/// <seealso href="https://learn.microsoft.com/en-us/dotnet/api/system.io.file.createhardlink?view=net-11.0" />
private static bool TryCreateHardLink(string path, string pathToTarget)
{
return CreateHardLink(pathToTarget, path, IntPtr.Zero);
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:353
- The new cache/hard-link behavior is a significant behavioral change (cache keying, hard-link fallback, and pruning). There are unit tests for basic shadow copying, but nothing here asserts that the cache is populated and reused (or that we gracefully fall back when hard links are unsupported). Consider adding a Windows-only test that (1) resolves the same analyzer path through two resolvers with the same base directory and asserts the cache file exists and is hard-linked (e.g., link count > 1 / file index matches), and (2) exercises the cache-pruning path with a small max limit to ensure it doesn’t throw.
// Optimization for antivirus scanning on Windows:
// - Shadow copied files are hard-linked to/from a cache directory if possible.
// - We continue to use per-session 'ShadowDirectory' for ease of implementing correct loading semantics and cleanup.
// - Hard linking a file from the cache instead of copying it is empirically observed to reduce time spent running AV scans when loading assemblies.
static void linkFromCacheOrFallbackToCopy(ShadowCopyAnalyzerPathResolver @this, string originalPath, string shadowCopyPath)
src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs:233
- The FILETIME ordering key sign-extends
dwHighDateTime(anint) when converting tolong, which can mis-order cache entries and evict the wrong files. Cast touintbefore widening so the comparison is based on the unsigned 64-bit FILETIME value.
This issue also appears in the following locations of the same file:
- line 349
- line 453
return (long)creationTime.dwHighDateTime << 32 | (uint)creationTime.dwLowDateTime;
src/Compilers/Core/Portable/FileSystem/FileUtilities.cs:13
- These
usingdirectives appear to be unused in this file (System.Runtime.InteropServices,System.Runtime.Versioning, andMicrosoft.Win32.SafeHandles). If warnings are treated as errors, this can break the build; otherwise it adds noise. Please remove them.
using System.Runtime.InteropServices;
using System.Runtime.Versioning;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.Win32.SafeHandles;
src/Compilers/Core/CodeAnalysisTest/ShadowCopyAnalyzerPathResolverTests.cs:10
System.LinqandSystem.Runtime.InteropServicesare unused in this test file. Removing them avoids unnecessary warnings/noise.
using System.Linq;
using System.Runtime.InteropServices;
using System.Runtime.Versioning;
Closes #13863. Specifically motivated by a recent comment on that issue. The original suggestion was likely around reducing file copies, which seems to be fairly negligible on modern nvme drives, even under decent load. The real overhead these days is in antivirus scanning.
I have some empirical evidence showing that this change improves Windows Defender behavior. It seems to indicate that Defender can reuse a scan result for different paths when they are hard links to each other. I performed the following steps, with and without my change:
Results without cache
Results with cache
Note that these particular FBA tests are just ones I've noticed being slow in the past, due to having a big defender penalty when loading solution-level analyzers. I think this is true for the LS tests, as well as startup of the real LS, in addition to analyzer loading from user projects. See also #82447 which was a separate strategy for addressing that in isolation.
With this change we could actually get rid of the
AsParallel()added by #82447. We now go faster than that with or without the parallelism.AsParallel()from Parallelize loading solution-level analyzers #82447 with no cache makes it take ~21s.I also added
AssemblyFileOperationBenchmarkswhich shows the following results on my machine:I still want to manually test putting temp directory on a Dev Drive where hard links don't work (IIRC). This change was implemented with the expectation that temp may be on a drive which doesn't support hard links, and everything needs to still work even if every attempt to hard-link fails. (Possibly allowing substituting a TryCreateHardLink function for testing, and trying an impl where that always fails, would be good.)
Microsoft Reviewers: Open in CodeFlow