dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.92k stars 4.02k forks source link

Linking System.Memory as an analyzer dependency can have a significant performance impact #58863

Open costin-zaharia-sonarsource opened 2 years ago

costin-zaharia-sonarsource commented 2 years ago

Version Used:

MSBuild version - 17.0.0-preview-21427-02+414393fc1 System.Memory - 4.6.27617.02

Context:

At SonarSource, our dotnet code analyzers depend on Google.Protobuf that depends on System.Memory. In order to ensure that System.Memory is available during compilation, we add a target file with an Analyzer element, similar with:

  <ItemGroup>
    <Analyzer Include="System.Memory.dll" />
  </ItemGroup>

I've noticed that just by adding this reference, the total build time increased significantly:

In both cases the build time more than doubles.

I've reproduced this behavior on two separate projects:

Steps to Reproduce:

  1. Checkout and build akka.net: dotnet build --no-incremental /v:m /p:reportanalyzer=true /bl:akka.binlog
  2. In akka.net add a new file Directory.Build.props with the following content:
    <Project>
    <ItemGroup>
    <Analyzer Include="<path>\System.Memory.dll" />
    </ItemGroup>
    </Project>
  3. Build the project again: dotnet build --no-incremental /v:m /p:reportanalyzer=true /bl:akka-with-link.binlog
  4. Observe the time difference: first run ~00:00:28.15, second run ~00:01:00.55.

  1. Checkout and build .Net Runtime: .\build.cmd "-restore -rebuild -c Release -subset libs -arch x64 -msbuild /bl:runtime.binlog"
  2. Update https://github.com/dotnet/runtime/blob/main/eng/Analyzers.props and add the following content:
    <ItemGroup>
    <Analyzer Include="<path>\System.Memory.dll" />
    </ItemGroup>
  3. Build the project again: .\build.cmd "-restore -rebuild -c Release -subset libs -arch x64 -msbuild /bl:runtime-with-link.binlog"
  4. Observe the time difference: first run ~00:04:09.06, second run ~00:12:36.17. This time the build time tripled.

Expected Behavior: Adding a reference to a dependency should not have a significant impact on the solution build time.

Actual Behavior: The build time more than doubled.

You can find below the bin logs for akka.net. For .Net Runtime they are too big and could not be attached: akka.zip akka-system.memory-linked.binlog.zip

sharwell commented 2 years ago

In order to ensure that System.Memory is available during compilation, we add a target file with an Analyzer element, similar with:

This dependency is provided by the compiler for Roslyn 3.0.0 and newer. If your analyzer does not target Roslyn 1.x or 2.x, there is no need to include the System.Memory dependency.

If there is a need for the System.Memory dependency, it should be excluded via build customization when the current runtime is .NET Core, otherwise it can end up using slow implementations designed for compatibility with .NET Framework.

costin-zaharia-sonarsource commented 2 years ago

Thanks for the comment. As we currently target Roslyn 1.3.2 (Visual Studio 2015 update 3) we will have to add this dependency only when really needed.

If there is a need for the System.Memory dependency, it should be excluded via build customization when the current runtime is .NET Core, otherwise it can end up using slow implementations designed for compatibility with .NET Framework.

Do I understand correctly that it is enough to check if the target runtime is .Net Core for excluding the dependency? or the safer approach is to look at the compiler version (>=3.0.0)?

sharwell commented 2 years ago

@costin-zaharia-sonarsource The performance issue is likely best mitigated by checking the target runtime.