Reduce File I/O under the AnalyzerAssemblyLoader folder #72412

ToddGrun · 2024-03-06T00:29:17Z

Assemblies and their corresponding resource dlls are copied/deleted under this folder on solution open. When opening Roslyn, I see about 700 dlls copied/deleted under this folder. Over 90% of these dlls are resource dlls, not something in use for my setup.

This change separates the code that ensures the assembly is shadow-copied properly, from the code that ensures the supporting resource assemblies are shadow-copied properly.

With this change there is a 90% reduction in the number of files shadow copied into the AnalyzerAssemblyLoader folder during roslyn solution load.

Did a test insertion with a speedometer run to make sure it did what I thought. Below images are from RichCopyTest.CopyPlain speedometer run, RoslynCodeAnalysisService process, FileIO/Create events for dlls filtered under AnalyzerAssemblyLoader folder

*** old (313 records):

*** new (106 records)

I also looked at the CompletionText.Completion speedometer data and it decreased from 227 records to 52.

…lyLoader folder. Assemblies and their corresponding resource dlls are copied/deleted under this folder on solution open. When opening Rosly, I see about 700 dlls copied/deleted under this folder. Over 90% of these dlls are resource dlls, not something in use for my setup. This change is very rough, but attempts to separate the code that ensures the assembly is shadow-copied properly, from the code that ensures that it's supporting resource assemblies are shadow-copied properly. With this change locally, I see over 90% reduction in the number of these file I/O operations.

ToddGrun · 2024-03-06T00:41:16Z

@jaredpar -- passed tests locally without any changes to the tests, we'll see what the ci says. Probably needs some cleanup, and I'm open to different ways to achieve this if you have ideas.

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.Desktop.cs

Added some comments Made culture check similar to old check in net core assembly resolver

ToddGrun · 2024-03-07T18:08:41Z

Elevating out of draft status

jaredpar · 2024-03-07T19:52:12Z

How will this change work for developers with non-English locales? I think the mentality for this change should be to assume that for a given build most analyzers will produce at least one diagnostic. On a non-English machine wouldn't this cause the satellite assembly load to trigger on every build where as for English it doesn't trigger? If so should we consider something like copying the satellite assemblies for CurrentUICulture in the initial analyzer copy? That doesn't get 100% of the cases since compiler switches can override CurrentUICulture but it would get the mainline cases.

ToddGrun · 2024-03-08T00:28:55Z

It might be best to chat in person for this as I'm not completely clear on the concerns. Both desktop/core AssemblyResolve paths call into GetSatelliteInfoForPath for resource assemblies. That method calls into GetSatelliteAssemblyInfoForPath which will only do the call to PrepareSatelliteAssembliesToLoad if the _analyzerSatelliteAssemblyInfoMap doesn't already contain that path.

There is a potential optimization here where only the resource assembly for the requested culture is prepared when requested instead of all cultures being prepared. I didn't go that route for simplicity concerns. As pointed out earlier, once you get into a multithreaded situation with differing write values, things can get tricky. Let me know if this is something you would like me to look into as part of this PR, or if I've completely misunderstood your concerns.

In reply to: 1984315411

ToddGrun · 2024-03-08T23:39:37Z

How will this change work for developers with non-English locales? I think the mentality for this change should be to assume that for a given build most analyzers will produce at least one diagnostic. On a non-English machine wouldn't this cause the satellite assembly load to trigger on every build where as for English it doesn't trigger? If so should we consider something like copying the satellite assemblies for CurrentUICulture in the initial analyzer copy? That doesn't get 100% of the cases since compiler switches can override CurrentUICulture but it would get the mainline cases.

Modified to individually copy over satellite assemblies as they are requested. Let me know whether this alleviates your concerns.

jaredpar · 2024-03-12T21:39:35Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/ShadowCopyAnalyzerAssemblyLoader.cs

+
+            var tcs = new TaskCompletionSource<bool>();
+            var task = _mvidSatelliteAssemblyPathMap.GetOrAdd((mvid, cultureName), tcs.Task);
+            if (object.ReferenceEquals(task, tcs.Task))


Non-blocking: The management of the Task here is virtually the same between the two methods. Feels like a case where we should be able to write it once and use it in both places. Don't have a lot of great recommendations though on best way to do this.

I had doubts, but the merging ended up being pretty clean after I changed the Task types to match

jaredpar · 2024-03-12T21:45:51Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

+            (satelliteAssemblyPath, var actualCultureName) = getSatelliteAssemblyLocation(originalAnalyzerPath, cultureInfo);
+            if (actualCultureName != null)
+            {
+                PrepareSatelliteAssemblyToLoad(originalAnalyzerPath, actualCultureName);


In this model it's possible that PrepareSatelliteAssemblyToLoad is called twice with the same arguments. Consider if only the ES culture assembly is present but we get calls to load es-es then es. Will the impl of this method be correct in the case it's called twice with same arguments?

I think the current code is fine. This code mimics how the non-satellite assembly is prepared via the call to PreparePathToLoad, as that could also be called twice with the same arg. The walk up the CultureInfo chain happens on the original path, which shouldn't be affected by any simultaneous file io occurring via PrepareSatelliteAssemblyToLoad.

jaredpar · 2024-03-13T23:18:18Z

@jjonescz, @RikkiGibson PTAL

jjonescz · 2024-03-14T10:04:52Z

This change separates the code that ensures the assembly is shadow-copied properly, from the code that ensures the supporting resource assemblies are shadow-copied properly.

How does separating shadow-copying the non-resource assembly from shadow-copying the resources assemblies reduce file I/O? I assume some copies are avoided, but I don't understand which ones.

ToddGrun · 2024-03-14T13:28:55Z

This change separates the code that ensures the assembly is shadow-copied properly, from the code that ensures the supporting resource assemblies are shadow-copied properly.

How does separating shadow-copying the non-resource assembly from shadow-copying the resources assemblies reduce file I/O? I assume some copies are avoided, but I don't understand which ones.

Previously, all resource assemblies were shadow copied (usually around 10 per assembly). Now, they are copied on-demand. If I run my machine in the default english locale, no resource assemblies are now copied. If an assembly resolve comes for a different locale for some reason, then that resource assembly is shadow copied over before returning it.

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.Desktop.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

RikkiGibson · 2024-03-15T17:44:05Z

It looks like there are legitimate test failures for the latest revision

ToddGrun · 2024-03-15T17:55:34Z

It looks like there are legitimate test failures for the latest revision

OK, think I fixed. Unit test code path wasn't changing an extension to indicate a resource assembly.

* upstream/main: (1045 commits) Fix excessive compilation times due to speculative parsing after an incomplete string (#72565) Convert official pipeline to 1ES template (#72430) Fix #69628 Analyzer summary should show suppressor ID (#72569) Fix event hookup even when in a projection buffer Drop win32-ia32 language server support Remove workaround for .net7 r2r assembly loading issue remove unused usings Share compilation when generators don't produce any generated files Fix potential exception in AssetProvider.SynchronizeAssetsAsync (#72597) Fix Update __arglist.md (#72523) Improve code gen for concatenation of `string` and `char` (#71793) Reduce File I/O under the AnalyzerAssemblyLoader folder (#72412) Reduce allocations in AbstractTypeMap (#72588) disable diagnostics when solution crawler option is disabled disable diagnostics when solution crawler option is disabled better thread transitions Fix Add check Fix ...

…et#72412)" This reverts commit 02069ce.

dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 6, 2024

jaredpar self-assigned this Mar 6, 2024

jaredpar added the Area-Compilers label Mar 6, 2024

jaredpar reviewed Mar 6, 2024

View reviewed changes

Cleanup around keeping different dictionaries for assembly/satellites

e74a7ac

Added some comments Made culture check similar to old check in net core assembly resolver

ToddGrun marked this pull request as ready for review March 7, 2024 18:09

ToddGrun requested review from a team as code owners March 7, 2024 18:09

ToddGrun changed the title ~~WIP (try2): Attempt at reducing the file I/O under the AnalyzerAssemblyLoader folder.~~ Reduce File I/O under the AnalyzerAssemblyLoader folder Mar 8, 2024

Individually copy over satellite assemblies as requested

bc97196

jaredpar reviewed Mar 12, 2024

View reviewed changes

Merge some logic that was very similar between the Prepare methods.

d1eef99

RikkiGibson self-assigned this Mar 14, 2024

jjonescz approved these changes Mar 15, 2024

View reviewed changes

Cleanup and clarify usage of real/original paths

75d8ca9

Fix unit test failure

6a72523

ToddGrun requested a review from jjonescz March 15, 2024 21:20

build-analysis bot mentioned this pull request Mar 15, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

jjonescz approved these changes Mar 18, 2024

View reviewed changes

jaredpar approved these changes Mar 18, 2024

View reviewed changes

ToddGrun merged commit 02069ce into dotnet:main Mar 19, 2024
30 checks passed

dotnet-policy-service bot added this to the Next milestone Mar 19, 2024

dotnet-bot mentioned this pull request Mar 21, 2024

[Automated] PRs inserted in VS build main-34720.100 #72629

Closed

RikkiGibson modified the milestones: Next, 17.10 P3 Mar 25, 2024

Cosifne added a commit to Cosifne/roslyn that referenced this pull request Nov 5, 2024

Revert "Reduce File I/O under the AnalyzerAssemblyLoader folder (dotn…

06e2bf6

…et#72412)" This reverts commit 02069ce.

Cosifne added a commit to Cosifne/roslyn that referenced this pull request Nov 5, 2024

Revert "Reduce File I/O under the AnalyzerAssemblyLoader folder (dotn…

2132105

…et#72412)" This reverts commit 02069ce.

Cosifne mentioned this pull request Nov 5, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce File I/O under the AnalyzerAssemblyLoader folder #72412

Reduce File I/O under the AnalyzerAssemblyLoader folder #72412

ToddGrun commented Mar 6, 2024 •

edited

Loading

ToddGrun commented Mar 6, 2024 •

edited

Loading

ToddGrun commented Mar 7, 2024

jaredpar commented Mar 7, 2024

ToddGrun commented Mar 8, 2024

ToddGrun commented Mar 8, 2024

jaredpar Mar 12, 2024

ToddGrun Mar 13, 2024

jaredpar Mar 12, 2024

ToddGrun Mar 13, 2024

jaredpar commented Mar 13, 2024

jjonescz commented Mar 14, 2024

ToddGrun commented Mar 14, 2024

RikkiGibson commented Mar 15, 2024

ToddGrun commented Mar 15, 2024 •

edited

Loading

Reduce File I/O under the AnalyzerAssemblyLoader folder #72412

Reduce File I/O under the AnalyzerAssemblyLoader folder #72412

Conversation

ToddGrun commented Mar 6, 2024 • edited Loading

ToddGrun commented Mar 6, 2024 • edited Loading

ToddGrun commented Mar 7, 2024

jaredpar commented Mar 7, 2024

ToddGrun commented Mar 8, 2024

ToddGrun commented Mar 8, 2024

jaredpar Mar 12, 2024

Choose a reason for hiding this comment

ToddGrun Mar 13, 2024

Choose a reason for hiding this comment

jaredpar Mar 12, 2024

Choose a reason for hiding this comment

ToddGrun Mar 13, 2024

Choose a reason for hiding this comment

jaredpar commented Mar 13, 2024

jjonescz commented Mar 14, 2024

ToddGrun commented Mar 14, 2024

RikkiGibson commented Mar 15, 2024

ToddGrun commented Mar 15, 2024 • edited Loading

ToddGrun commented Mar 6, 2024 •

edited

Loading

ToddGrun commented Mar 6, 2024 •

edited

Loading

ToddGrun commented Mar 15, 2024 •

edited

Loading