Skip to content

Comments

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

Open
Cosifne wants to merge 6 commits intodotnet:mainfrom
Cosifne:dev/shech/revert72412In17.12
Open

Revert "Reduce File I/O under the AnalyzerAssemblyLoader folder (#724…#75761
Cosifne wants to merge 6 commits intodotnet:mainfrom
Cosifne:dev/shech/revert72412In17.12

Conversation

@Cosifne
Copy link
Member

@Cosifne Cosifne commented Nov 5, 2024

@ghost ghost added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 5, 2024
@Cosifne
Copy link
Member Author

Cosifne commented Nov 6, 2024

I think I need @ToddGrun and @jaredpar 's review as I don't catch all the details of the original PR.

@Cosifne Cosifne marked this pull request as ready for review November 6, 2024 00:29
@Cosifne Cosifne requested review from a team as code owners November 6, 2024 00:29
AssemblyName? bestName = null;
foreach (var candidateOriginalPath in paths.OrderBy(StringComparer.Ordinal))
{
(AssemblyName? candidateName, string candidateRealPath) = GetAssemblyInfoForPath(candidateOriginalPath);
Copy link
Member Author

@Cosifne Cosifne Nov 6, 2024

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

@ToddGrun
Copy link
Contributor

ToddGrun commented Nov 7, 2024

Have you done a test insertion with this change to see if there are any noticeable perf regressions?

@Cosifne Cosifne changed the base branch from release/dev17.12 to main December 2, 2024 18:47
@JoeRobich
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@arunchndr arunchndr enabled auto-merge January 30, 2025 19:20
@arunchndr arunchndr disabled auto-merge January 30, 2025 19:52
@ToddGrun
Copy link
Contributor

ToddGrun commented Feb 3, 2025

@jaredpar -- is there an alternative branch/PR you are working on for this that I can take a look at out of curioisity?

@jaredpar
Copy link
Member

jaredpar commented Feb 3, 2025

@ToddGrun

#77004

This PR moves the analyzer loading behaviors into two interfaces that can be composed. The big change there is that shadow copying no longer has an inheritance relationship anymore but instead is a composable component. That makes it a lot easier to customize it. For example can have the VS layer turn on "eagerly shadow copy resources" and leave that off everywhere else.

The PR is not done yet, still working on the details and haven't pushed much customization in the VS layer but you can see the overall shape I'm going for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead VSCode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants