Skip to content
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.

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

Expression.Compile performance regression on .NET7 RC1 #75891

Closed
KeterSCP opened this issue Sep 20, 2022 · 12 comments
Closed

Expression.Compile performance regression on .NET7 RC1 #75891

KeterSCP opened this issue Sep 20, 2022 · 12 comments
Labels
area-VM-coreclr tenet-performance Performance related issue
Milestone

Comments

@KeterSCP
Copy link

KeterSCP commented Sep 20, 2022

Description

Running simple benchmark to test performance of Expression.Compile shows regressions on .NET7 RC1

Benchmark code
using System;
using System.Linq.Expressions;

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using BenchmarkDotNet.Jobs;
using BenchmarkDotNet.Running;

BenchmarkRunner.Run<Benchmarks>();

[MemoryDiagnoser(false)]
[SimpleJob(RuntimeMoniker.Net60)]
[SimpleJob(RuntimeMoniker.Net70)]
[GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByJob)]
public class Benchmarks
{
  private static readonly Expression<Func<MyClass>> _factory = FactoryCreator<MyClass>.GetFactory();

  [Benchmark]
  public Func<MyClass> CompileFactory()
  {
      return _factory.Compile();
  }
}

public static class FactoryCreator<T>
{
  public static Expression<Func<T>> GetFactory()
  {
      var ctorBody = Expression.New(typeof(T).GetConstructor(Type.EmptyTypes)!);

      return Expression.Lambda<Func<T>>(ctorBody);
  }
}

public class MyClass {}

Regression?

Yes. On .NET 6 this is ~50% faster

Data

BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22000.978/21H2)
AMD Ryzen 5 3600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=7.0.100-rc.1.22431.12
  [Host]   : .NET 7.0.0 (7.0.22.42610), X64 RyuJIT AVX2
  .NET 6.0 : .NET 6.0.9 (6.0.922.41905), X64 RyuJIT AVX2
  .NET 7.0 : .NET 7.0.0 (7.0.22.42610), X64 RyuJIT AVX2

Method Job Runtime Mean Error StdDev Allocated
CompileFactory .NET 6.0 .NET 6.0 33.51 μs 0.361 μs 0.337 μs 3.66 KB
CompileFactory .NET 7.0 .NET 7.0 51.86 μs 0.223 μs 0.208 μs 3.69 KB
@KeterSCP KeterSCP added the tenet-performance Performance related issue label Sep 20, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 20, 2022
@EgorBo
Copy link
Member

EgorBo commented Sep 20, 2022

Reproduces on RC1 but doesn't reproduce on Main

@KeterSCP
Copy link
Author

@EgorBo does it mean that in the final .NET7 release this will be fixed? If so I can close the issue

@EgorBo
Copy link
Member

EgorBo commented Sep 20, 2022

@EgorBo does it mean that in the final .NET7 release this will be fixed? If so I can close the issue

No, Main is .NET8, we need to check this on the latest .NET 7.0 daily build
Perhaps this issue rings a bell to anyone? It seems to be in either coreclr.dll or clrjit.dll (I replaced those in net7.0 by net8.0 binaries and got the results back)

@jeffschwMSFT
Copy link
Member

Can you try the benchmark with COMPlus_EnableWriteXorExecute=0? We have a fix for a similar situation in RC2

cc @mangod9

@KeterSCP
Copy link
Author

Can you try the benchmark with COMPlus_EnableWriteXorExecute=0?

Yes, I can confirm disabling W^X fixes the regression:

Method Job EnvironmentVariables Runtime Mean Error StdDev
CompileFactory .NET 6 Empty .NET 6.0 34.92 μs 0.319 μs 0.283 μs
CompileFactory .NET 7 Empty .NET 7.0 51.46 μs 0.338 μs 0.316 μs
CompileFactory .NET 7 with W^X disabled COMPlus_EnableWriteXorExecute=0 .NET 7.0 28.20 μs 0.231 μs 0.216 μs

Feel free to close the issue if it will be fixed in the RC2

@mangod9 mangod9 removed the untriaged New issue has not been triaged by the area owner label Sep 21, 2022
@mangod9 mangod9 added this to the 7.0.0 milestone Sep 21, 2022
@jeffschwMSFT
Copy link
Member

It would be good to validate with RC2. The fix we have in RC2 is to cache the W^X mapping, so rapid use of pages (like this micro benchmark) should notice a benefit.

@mangod9
Copy link
Member

mangod9 commented Sep 21, 2022

marking for 7 for now till we confirm its fixed in RC2. @KeterSCP can you please try a build from rc2 installer repo: https://github.com/dotnet/installer/tree/release/7.0.1xx

@KeterSCP
Copy link
Author

@mangod9 could you please tell me how can I try the RC2 build? should I build it from source or there are already built installers?

@mangod9
Copy link
Member

mangod9 commented Sep 21, 2022

yeah you can use the installer link listed here: https://github.com/dotnet/installer/tree/release/7.0.1xx#table. Just note that daily builds are not signed

@KeterSCP
Copy link
Author

Thanks for the tip @mangod9! I can confirm that regression was fixed on the latest .NET7 build:

Method Job EnvironmentVariables Mean Error StdDev Allocated
CompileFactory .NET 7 Empty 32.61 μs 0.247 μs 0.231 μs 3.69 KB
CompileFactory .NET 7 with disabled W^X COMPlus_EnableWriteXorExecute=0 30.67 μs 0.325 μs 0.304 μs 3.69 KB

@mangod9
Copy link
Member

mangod9 commented Sep 21, 2022

Thanks for confirming @KeterSCP. Closing this issue then.

@mangod9 mangod9 closed this as completed Sep 21, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

4 participants