Skip to content

Revert "Reduce File I/O under the AnalyzerAssemblyLoader folder (#724… #75761

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

Open
Cosifne wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Revert "Reduce File I/O under the AnalyzerAssemblyLoader folder (#724… #75761

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2132105
Revert "Reduce File I/O under the AnalyzerAssemblyLoader folder (#724…
Cosifne Nov 5, 2024
1d09104
Fix unresolved conflict marks
Cosifne Nov 5, 2024
7ac7221
Remove dup imports
Cosifne Nov 5, 2024
a34cfc5
Keep the GetBestPathHelper
Cosifne Nov 5, 2024
c3f09b3
bring AssemblyLoading_ResourcesInParent(AnalyzerTestKind kind) back
Cosifne Nov 6, 2024
8383853
Add blank line
Cosifne Nov 7, 2024
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
28 changes: 4 additions & 24 deletions src/Compilers/Core/CodeAnalysisTest/AnalyzerAssemblyLoaderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,21 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Text;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Test.Utilities;
using Roslyn.Test.Utilities;
using Roslyn.Utilities;
using Xunit;
using Xunit.Abstractions;
using Xunit.Sdk;
using Microsoft.CodeAnalysis.VisualBasic;

#if NET
Expand Down Expand Up @@ -368,32 +370,11 @@ string getExpectedLoadPath(string path)

if (path.EndsWith(".resources.dll", StringComparison.Ordinal))
{
return getRealSatelliteLoadPath(path) ?? "";
return loader.GetRealSatelliteLoadPath(path) ?? "";
}
return loader.GetRealAnalyzerLoadPath(path ?? "");
}

// When PreparePathToLoad is overridden this returns the most recent
// real path for the given analyzer satellite assembly path
string? getRealSatelliteLoadPath(string originalSatelliteFullPath)
{
// This is a satellite assembly, need to find the mapped path of the real assembly, then
// adjust that mapped path for the suffix of the satellite assembly
//
// Example of dll and it's corresponding satellite assembly
//
// c:\some\path\en-GB\util.resources.dll
// c:\some\path\util.dll
var assemblyFileName = Path.ChangeExtension(Path.GetFileNameWithoutExtension(originalSatelliteFullPath), ".dll");

var assemblyDir = Path.GetDirectoryName(originalSatelliteFullPath)!;
var cultureInfo = CultureInfo.GetCultureInfo(Path.GetFileName(assemblyDir));
assemblyDir = Path.GetDirectoryName(assemblyDir)!;

// Real assembly is located in the directory above this one
var assemblyPath = Path.Combine(assemblyDir, assemblyFileName);
return loader.GetRealSatelliteLoadPath(assemblyPath, cultureInfo);
}
}

private static void VerifyAssemblies(AnalyzerAssemblyLoader loader, IEnumerable<Assembly> assemblies, int? copyCount, params string[] assemblyPaths)
Expand Down Expand Up @@ -1467,7 +1448,6 @@ public void AssemblyLoading_ResourcesInParent(AnalyzerTestKind kind)
VerifyDependencyAssemblies(loader, copyCount: 1, analyzerPath, analyzerResourcesPath);
});
}
Comment thread
Cosifne marked this conversation as resolved.

Comment thread
Cosifne marked this conversation as resolved.
#if NET

[Theory]
Expand Down
11 changes: 5 additions & 6 deletions src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.Core.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ public DirectoryLoadContext(string directory, AnalyzerAssemblyLoader loader)
var assemblyPath = Path.Combine(Directory, simpleName + ".dll");
if (_loader.IsAnalyzerDependencyPath(assemblyPath))
{
(_, var loadPath) = _loader.GetAssemblyInfoForPath(assemblyPath);
(_, var loadPath, _) = _loader.GetAssemblyInfoForPath(assemblyPath);
return loadCore(loadPath);
}

Expand All @@ -156,11 +156,11 @@ public DirectoryLoadContext(string directory, AnalyzerAssemblyLoader loader)
// loader has a mode where it loads from Stream though and the runtime will not handle
// that automatically. Rather than bifurcate our loading behavior between Disk and
// Stream both modes just handle satellite loading directly
if (assemblyName.CultureInfo is not null && simpleName.EndsWith(".resources", StringComparison.Ordinal))
if (!string.IsNullOrEmpty(assemblyName.CultureName) && simpleName.EndsWith(".resources", StringComparison.Ordinal))
{
var analyzerFileName = Path.ChangeExtension(simpleName, ".dll");
var analyzerFilePath = Path.Combine(Directory, analyzerFileName);
var satelliteLoadPath = _loader.GetRealSatelliteLoadPath(analyzerFilePath, assemblyName.CultureInfo);
var satelliteLoadPath = _loader.GetSatelliteInfoForPath(analyzerFilePath, assemblyName.CultureName);
if (satelliteLoadPath is not null)
{
return loadCore(satelliteLoadPath);
Expand All @@ -173,8 +173,7 @@ public DirectoryLoadContext(string directory, AnalyzerAssemblyLoader loader)
// be necessary but msbuild target defaults have caused a number of customers to
// fall into this path. See discussion here for where it comes up
// https://github.com/dotnet/roslyn/issues/56442
var (_, bestRealPath) = _loader.GetBestPath(assemblyName);
if (bestRealPath is not null)
if (_loader.GetBestPath(assemblyName).BestRealPath is string bestRealPath)
{
return loadCore(bestRealPath);
}
Expand All @@ -201,7 +200,7 @@ protected override IntPtr LoadUnmanagedDll(string unmanagedDllName)
var assemblyPath = Path.Combine(Directory, unmanagedDllName + ".dll");
if (_loader.IsAnalyzerDependencyPath(assemblyPath))
{
(_, var loadPath) = _loader.GetAssemblyInfoForPath(assemblyPath);
(_, var loadPath, _) = _loader.GetAssemblyInfoForPath(assemblyPath);
return LoadUnmanagedDllFromPath(loadPath);
}

Expand Down
25 changes: 3 additions & 22 deletions src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.Desktop.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,30 +117,11 @@ internal bool EnsureResolvedUnhooked()
{
try
{
const string resourcesExtension = ".resources";
var assemblyName = new AssemblyName(args.Name);
var simpleName = assemblyName.Name;
var isSatelliteAssembly =
assemblyName.CultureInfo is not null &&
simpleName.EndsWith(resourcesExtension, StringComparison.Ordinal);

if (isSatelliteAssembly)
{
// Satellite assemblies should get the best path information using the
// non-resource part of the assembly name. Once the path information is obtained
// GetSatelliteInfoForPath will translate to the resource assembly path.
assemblyName.Name = simpleName[..^resourcesExtension.Length];
}

var (originalPath, realPath) = GetBestPath(assemblyName);
if (isSatelliteAssembly && originalPath is not null)
{
realPath = GetRealSatelliteLoadPath(originalPath, assemblyName.CultureInfo);
}

if (realPath is not null)
string? bestPath = GetBestPath(assemblyName).BestRealPath;
if (bestPath is not null)
{
return Assembly.LoadFrom(realPath);
return Assembly.LoadFrom(bestPath);
}

return null;
Expand Down
143 changes: 69 additions & 74 deletions src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Reflection;
Expand Down Expand Up @@ -48,17 +47,7 @@ internal abstract partial class AnalyzerAssemblyLoader : IAnalyzerAssemblyLoader
/// <remarks>
/// Access must be guarded by <see cref="_guard"/>
/// </remarks>
private readonly Dictionary<string, (AssemblyName? AssemblyName, string RealAssemblyPath)?> _analyzerAssemblyInfoMap = new();

/// <summary>
/// Mapping of analyzer dependency original full path and culture to the real satellite
/// assembly path. If the satellite assembly doesn't exist for the original analyzer and
/// culture, the real path value stored will be null.
/// </summary>
/// <remarks>
/// Access must be guarded by <see cref="_guard"/>
/// </remarks>
private readonly Dictionary<(string OriginalAnalyzerPath, CultureInfo CultureInfo), string?> _analyzerSatelliteAssemblyRealPaths = new();
private readonly Dictionary<string, (AssemblyName? AssemblyName, string RealAssemblyPath, ImmutableHashSet<string> SatelliteCultureNames)?> _analyzerAssemblyInfoMap = new();

/// <summary>
/// Maps analyzer dependency simple names to the set of original full paths it was loaded from. This _only_
Expand Down Expand Up @@ -149,7 +138,7 @@ public void AddDependencyLocation(string fullPath)
_knownAssemblyPathsBySimpleName[simpleName] = paths.Add(fullPath);
}

// This type assumes the file system is static for the duration of the
// This type assumses the file system is static for the duration of the
// it's instance. Repeated calls to this method, even if the underlying
// file system contents, should reuse the results of the first call.
_ = _analyzerAssemblyInfoMap.TryAdd(fullPath, null);
Expand All @@ -162,7 +151,7 @@ public Assembly LoadFromPath(string originalAnalyzerPath)

CompilerPathUtilities.RequireAbsolutePath(originalAnalyzerPath, nameof(originalAnalyzerPath));

(AssemblyName? assemblyName, _) = GetAssemblyInfoForPath(originalAnalyzerPath);
(AssemblyName? assemblyName, _, _) = GetAssemblyInfoForPath(originalAnalyzerPath);

// Not a managed assembly, nothing else to do
if (assemblyName is null)
Expand All @@ -189,7 +178,7 @@ public Assembly LoadFromPath(string originalAnalyzerPath)
/// because we only want information for registered paths. Using unregistered paths inside the
/// implementation should result in errors.
/// </remarks>
protected (AssemblyName? AssemblyName, string RealAssemblyPath) GetAssemblyInfoForPath(string originalAnalyzerPath)
protected (AssemblyName? AssemblyName, string RealAssemblyPath, ImmutableHashSet<string> SatelliteCultureNames) GetAssemblyInfoForPath(string originalAnalyzerPath)
{
CheckIfDisposed();

Expand All @@ -206,7 +195,8 @@ public Assembly LoadFromPath(string originalAnalyzerPath)
}
}

string realPath = PreparePathToLoad(originalAnalyzerPath);
var resourceAssemblyCultureNames = getResourceAssemblyCultureNames(originalAnalyzerPath);
string realPath = PreparePathToLoad(originalAnalyzerPath, resourceAssemblyCultureNames);
AssemblyName? assemblyName;
try
{
Expand All @@ -222,68 +212,35 @@ public Assembly LoadFromPath(string originalAnalyzerPath)

lock (_guard)
{
_analyzerAssemblyInfoMap[originalAnalyzerPath] = (assemblyName, realPath);
_analyzerAssemblyInfoMap[originalAnalyzerPath] = (assemblyName, realPath, resourceAssemblyCultureNames);
}

return (assemblyName, realPath);
}
return (assemblyName, realPath, resourceAssemblyCultureNames);

/// <summary>
/// Get the path a satellite assembly should be loaded from for the given original
/// analyzer path and culture
/// </summary>
/// <remarks>
/// This is used during assembly resolve for satellite assemblies to determine the
/// path from where the satellite assembly should be loaded for the specified culture.
/// This method calls <see cref="PrepareSatelliteAssemblyToLoad"/> to ensure this path
/// contains the satellite assembly.
/// </remarks>
internal string? GetRealSatelliteLoadPath(string originalAnalyzerPath, CultureInfo cultureInfo)
{
CheckIfDisposed();

string? realSatelliteAssemblyPath = null;

lock (_guard)
// Discover the culture names for any satellite dlls related to this analyzer. These
// need to be understood when handling the resource loading in certain cases.
static ImmutableHashSet<string> getResourceAssemblyCultureNames(string originalAnalyzerPath)
{
if (_analyzerSatelliteAssemblyRealPaths.TryGetValue((originalAnalyzerPath, cultureInfo), out realSatelliteAssemblyPath))
var path = Path.GetDirectoryName(originalAnalyzerPath)!;
using var enumerator = Directory.EnumerateDirectories(path, "*").GetEnumerator();
if (!enumerator.MoveNext())
{
return realSatelliteAssemblyPath;
return ImmutableHashSet<string>.Empty;
}
}

var actualCultureName = getSatelliteCultureName(originalAnalyzerPath, cultureInfo);
if (actualCultureName != null)
{
realSatelliteAssemblyPath = PrepareSatelliteAssemblyToLoad(originalAnalyzerPath, actualCultureName);
}

lock (_guard)
{
_analyzerSatelliteAssemblyRealPaths[(originalAnalyzerPath, cultureInfo)] = realSatelliteAssemblyPath;
}

return realSatelliteAssemblyPath;

// Discover the most specific culture name to use for the specified analyzer path and culture
static string? getSatelliteCultureName(string originalAnalyzerPath, CultureInfo cultureInfo)
{
var path = Path.GetDirectoryName(originalAnalyzerPath)!;
var resourceFileName = GetSatelliteFileName(Path.GetFileName(originalAnalyzerPath));

while (cultureInfo != CultureInfo.InvariantCulture)
var builder = ImmutableHashSet.CreateBuilder<string>(StringComparer.OrdinalIgnoreCase);
do
{
var resourceFilePath = Path.Combine(path, cultureInfo.Name, resourceFileName);

var resourceFilePath = Path.Combine(enumerator.Current, resourceFileName);
if (File.Exists(resourceFilePath))
{
return cultureInfo.Name;
builder.Add(Path.GetFileName(enumerator.Current));
}

cultureInfo = cultureInfo.Parent;
}
while (enumerator.MoveNext());

return null;
return builder.ToImmutableHashSet();
}
}

Expand All @@ -293,6 +250,23 @@ public Assembly LoadFromPath(string originalAnalyzerPath)

return GetBestPath(assemblyName).BestOriginalPath;
}
/// <summary>
/// Get the real load path of the satellite assembly given the original path to the analyzer
/// and the desired culture name.
/// </summary>
protected string? GetSatelliteInfoForPath(string originalAnalyzerPath, string cultureName)
{
var (_, realAssemblyPath, satelliteCultureNames) = GetAssemblyInfoForPath(originalAnalyzerPath);
if (!satelliteCultureNames.Contains(cultureName))
{
return null;
}

var satelliteFileName = GetSatelliteFileName(Path.GetFileName(realAssemblyPath));
var dir = Path.GetDirectoryName(realAssemblyPath)!;
return Path.Combine(dir, cultureName, satelliteFileName);
}

/// <summary>
/// Return the best (original, real) path information for loading an assembly with the specified <see cref="AssemblyName"/>.
/// </summary>
Expand Down Expand Up @@ -321,7 +295,7 @@ public Assembly LoadFromPath(string originalAnalyzerPath)
AssemblyName? bestName = null;
foreach (var candidateOriginalPath in paths.OrderBy(StringComparer.Ordinal))
{
(AssemblyName? candidateName, string candidateRealPath) = GetAssemblyInfoForPath(candidateOriginalPath);

@Cosifne Cosifne Nov 6, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I keep GetBestPath change from Todd's PR as I see later this method (line 290)

        public string? GetOriginalDependencyLocation(AssemblyName assemblyName)
        {
            CheckIfDisposed();

            return GetBestPath(assemblyName).BestOriginalPath;
        }

is referencing the BestOriginalPath

(AssemblyName? candidateName, string candidateRealPath, _) = GetAssemblyInfoForPath(candidateOriginalPath);
if (candidateName is null)
{
continue;
Expand Down Expand Up @@ -353,18 +327,15 @@ protected static string GetSatelliteFileName(string assemblyFileName) =>
/// When overridden in a derived class, allows substituting an assembly path after we've
/// identified the context to load an assembly in, but before the assembly is actually
/// loaded from disk. This is used to substitute out the original path with the shadow-copied version.
///
/// In the case the <param name="assemblyFilePath" /> is moved to a new location then
/// the resource DLLs for the specified <paramref name="resourceAssemblyCultureNames"/> must also be
/// moved _but_ retain their original relative location.
/// </summary>
protected abstract string PreparePathToLoad(string assemblyFilePath);

/// <summary>
/// When overridden in a derived class, allows substituting a satellite assembly path after we've
/// identified the context to load a satellite assembly in, but before the satellite assembly is actually
/// loaded from disk. This is used to substitute out the original path with the shadow-copied version.
/// </summary>
protected abstract string PrepareSatelliteAssemblyToLoad(string assemblyFilePath, string cultureName);
protected abstract string PreparePathToLoad(string assemblyFilePath, ImmutableHashSet<string> resourceAssemblyCultureNames);

/// <summary>
/// When <see cref="PreparePathToLoad(string)"/> is overridden this returns the most recent
/// When <see cref="PreparePathToLoad(string, ImmutableHashSet{string})"/> is overridden this returns the most recent
/// real path calculated for the <paramref name="originalFullPath"/>
/// </summary>
internal string GetRealAnalyzerLoadPath(string originalFullPath)
Expand All @@ -382,6 +353,30 @@ internal string GetRealAnalyzerLoadPath(string originalFullPath)
}
}

/// <summary>
/// When <see cref="PreparePathToLoad(string, ImmutableHashSet{string})"/> is overridden this returns the most recent
/// real path for the given analyzer satellite assembly path
/// </summary>
internal string? GetRealSatelliteLoadPath(string originalSatelliteFullPath)
{
// This is a satellite assembly, need to find the mapped path of the real assembly, then
// adjust that mapped path for the suffix of the satellite assembly
//
// Example of dll and it's corresponding satellite assembly
//
// c:\some\path\en-GB\util.resources.dll
// c:\some\path\util.dll
var assemblyFileName = Path.ChangeExtension(Path.GetFileNameWithoutExtension(originalSatelliteFullPath), ".dll");

var assemblyDir = Path.GetDirectoryName(originalSatelliteFullPath)!;
var cultureName = Path.GetFileName(assemblyDir);
assemblyDir = Path.GetDirectoryName(assemblyDir)!;

// Real assembly is located in the directory above this one
var assemblyPath = Path.Combine(assemblyDir, assemblyFileName);
return GetSatelliteInfoForPath(assemblyPath, cultureName);
}

internal (string OriginalAssemblyPath, string RealAssemblyPath)[] GetPathMapSnapshot()
{
CheckIfDisposed();
Expand Down
Loading