Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Sep 8, 2022

We noticed that SplitNonFoundationalTcImports is causing a spawn of dotnet to find the installed .NET SDKs, in order to determine which assembly references are "system" assemblies.

As a short term fix for that problem I'd say it's sufficient just to look for "dotnet/packs" or dotnet/packs/Microsoft.NETCore.App.Ref in the normalized path of the assembly reference. But these are not good solutions.

Instead I wonder if we should just remove this "foundational/framework TcImports" altogether. This thing adds a lot of complexity to the code and loads of edge cases would disappear if we removed it.

As background, the aim of that object is to try to share resources related to common referenced assemblies in projects that have the full set of SDK references in common. For this to work, we must have exactly the same set of "system" references shared between two loaded projects. So it relies heavily on a heuristic about what is a "system" reference.

This seemed useful in the days of .NET 1.0-2.0, when all projects would tend to reference the whole .NET framework from the installed location. Then gradually reference assemblies etc. came in and I suspect it's really rare that two projects share the same foundational imports. I guess we could measure that.

However there are caveats

  • It may be much less rare that two scripts share the same set of imports - it would almost be normal. So scenarios like having 300 scripts open in the IDE need to be considered. However again we should measure if we're getting any actual important reduction in memory usage - and perhaps we should just rely on the other resource-sharing-and-reduction mechanisms we have in place.

  • Our tests certainly repeatedly share the same foundational imports. This is probably actually the biggest problem.

Note many of the resources associated with referenced binaries are shared/held-weakly in ilModuleReaderCache1 and ilModuleReaderCache2 . However this doesn't include the resources associated with converting ILModule --> Ccu, i.e. the TypedTree nodes created on-demand via ImportILAssembly, which are really the only things we're trying to share here.

Here I've spiked the total removal of foundational/framework TcImports.

  • Check run times of tests
  • Check VS memory usage when 200 scripts are loaded and each has been analysed

@dsyme
Copy link
Contributor Author

dsyme commented Sep 8, 2022

From the failing tests it looks like I busted in-memory project references in IncrementalBuilder.fs - need to take a closer look

@vzarytovskii vzarytovskii marked this pull request as draft October 17, 2022 08:42
@dsyme dsyme closed this Oct 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

1 participant