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

Use DependencyContext to load diagnostics assembly #958

Merged
merged 2 commits into from Nov 15, 2018

Conversation

Projects
None yet
4 participants
@wojtpl2
Copy link
Collaborator

wojtpl2 commented Nov 14, 2018

Fix for #955

@adamsitnik
Copy link
Member

adamsitnik left a comment

@wojtpl2 big thanks for help!

could you please answer my question before I merge it?

@@ -96,10 +97,16 @@ private static IDiagnoser[] LoadClassic()

private static Assembly LoadDiagnosticsAssembly(Assembly benchmarkDotNetCoreAssembly)
{
// We couldn't use Assembly.GetEntryAssembly()?.GetReferencedAssemblies() because

This comment has been minimized.

@adamsitnik

adamsitnik Nov 14, 2018

Member

I think that we should keep the old way (to make it work for old .NET frameworks) and extend it with the new one.

So it would be:

var referencedAssemblyName = Assembly.GetEntryAssembly()?.GetReferencedAssemblies().SingleOrDefault(name => name.Name == DiagnosticAssemblyName);	            if (referencedAssemblyName != default(AssemblyName))
    return Assembly.Load(referencedAssemblyName);

var inCompileLibraries = DependencyContext.Default.CompileLibraries.Any(l => l.Name.Equals(DiagnosticAssemblyName, StringComparison.OrdinalIgnoreCase));
var inRuntimeLibraries = DependencyContext.Default.RuntimeLibraries.Any(l => l.Name.Equals(DiagnosticAssemblyName, StringComparison.OrdinalIgnoreCase));

if (inCompileLibraries || inRuntimeLibraries)
    return Assembly.Load(new AssemblyName(DiagnosticAssemblyName));

This comment has been minimized.

@wojtpl2

wojtpl2 Nov 14, 2018

Collaborator

I'll check old .Net frameworks path.

This comment has been minimized.

@eerhardt

eerhardt Nov 14, 2018

Member

Agreed. DependencyContext doesn't work "by default" on .NET Framework. DependencyContext is mostly just for the .NET Core case.

You should also check for DependencyContext.Default == null, which will be the case when a .deps.json file isn't present.


if (inCompileLibraries || inRuntimeLibraries)
{
return Assembly.Load(new AssemblyName(DiagnosticAssemblyName));

This comment has been minimized.

@adamsitnik

adamsitnik Nov 14, 2018

Member

I would assume that the DependencyContext exposes some method to load the library without providing the name. Here you just check that it's on the list and if it is you load it by name. How Assembly.Load will know about the .deps file here?

@eerhardt how should we use DependencyContext to load an assembly?

This comment has been minimized.

@wojtpl2

wojtpl2 Nov 14, 2018

Collaborator

I found this way there dotnet/corefx#11639 (comment).
I created project on the side and this solution works.

I don't know how Assembly.Load works. I'll try to check.

This comment has been minimized.

@eerhardt

eerhardt Nov 14, 2018

Member

DependencyContext allows you to get the information about a dependency, but it doesn't have assembly loading capabilities. You still use the normal Assembly.Load APIs to load the assembly.

The reason I suggested using DependencyContext is because I assumed the main thing you need/want here is you want to get the AssemblyVersion of the referenced BenchmarkDotNet.Diagnostics.Windows.dll, right? (That information can be found on this API).

If it works to just Assembly.Load(new AssemblyName(DiagnosticAssemblyName));, then you don't need the DependencyContext nor the bit about Assembly.GetEntryAssembly()?.GetReferencedAssemblies() at all. Just call Assembly.Load passing in the .dll name.

This comment has been minimized.

@wojtpl2

wojtpl2 Nov 15, 2018

Collaborator

Calling Assembly.Load is enough for .net core and full framework.
For full framework, it works even if instead of referencing a library, I copy the dll to the bin directory.

if (referencedAssemblyName != default(AssemblyName))
return Assembly.Load(referencedAssemblyName);
// So we use DependencyContext that reads *.deps.json file that contains all referenced assembly.
var inCompileLibraries = DependencyContext.Default.CompileLibraries.Any(l => l.Name.Equals(DiagnosticAssemblyName, StringComparison.OrdinalIgnoreCase));

This comment has been minimized.

@eerhardt

eerhardt Nov 14, 2018

Member

You shouldn't check the CompileLibraries at all. These are not appropriate for runtime scenarios, and are mainly there to support things like ASP.NET MVC Razor, which compiles code at runtime using Roslyn.

@wojtpl2

This comment has been minimized.

Copy link
Collaborator

wojtpl2 commented Nov 15, 2018

@eerhardt @adamsitnik Big thanks for this review. It is amazing how much I can learn in this project. I'll fix this PR..

@wojtpl2

This comment has been minimized.

Copy link
Collaborator

wojtpl2 commented Nov 15, 2018

As @eerhardt wrote, BDN doesn't need DependencyContext at all. I tested this solution for .net core and full framework.

@adamsitnik

This comment has been minimized.

Copy link
Member

adamsitnik commented Nov 15, 2018

@wojtpl2, unfortunately, it needs it. The difference is when you work with our local project built on your PC (when everything is in bin) vs BDN installed via NuGet (when BDN is in NuGet folder sth like C:\Users\adsitnik.nuget\packages)

The problem that we try to solve is the following:

  1. User installs BenchmarkDotNet.Diagnostics.Windows NuGet package but does not consume any of the types defined in this package.
  2. User uses BenchmarkSwitcher and passes args to it
  3. User provides --profiler ETW command line argument, the way we use Assembly.Load today fails from loading BenchmarkDotNet.Diagnostics.Windows because it's not in the bin folder. Here is where we need to use DependencyContext

Repro:

dotnet run -c Release -- --filter * --profiler ETW
class Program
{
    static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);
}

public class Sample
{
    [Benchmark] public void TheMethod() { }  
}

Reproducing this issue is not easy. The best way to do it know to me:

  1. Use dotnet publish and publish a BenchmarkDotNet NuGet packages
  2. Copy the packages to a folder
  3. Create new console app (dotnet new console)
  4. Create new nuget.config file (dotnet new nugetconfig), add the folder where you store the packages as a feed to the list in this nuget.config file
  5. Install BDN from the local feed
  6. Run the sample I provided above

Once you are able to repro the problem locally and provide a fix you need to remove the local NuGet from C:\Users\$username\.nuget\packages and reinstall the new version. If you try to install a NuGet package with same version, but different content NuGet client won't update the things stored in C:\Users$username.nuget\packages

@wojtpl2

This comment has been minimized.

Copy link
Collaborator

wojtpl2 commented Nov 15, 2018

I've reproducing it in different way:
I have a project:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFrameworks>net4.7.2;netcoreapp2.1</TargetFrameworks>
  </PropertyGroup>

  <ItemGroup>
    <!-- <PackageReference Include="BenchmarkDotNet" Version="0.11.2" />  -->
    <PackageReference Include="BenchmarkDotNet.Diagnostics.Windows" Version="0.11.2" />
    <PackageReference Include="Microsoft.Diagnostics.Tracing.TraceEvent" Version="2.0.26" />
  </ItemGroup>

  <ItemGroup>
    <ProjectReference Include="..\src\BenchmarkDotNet\BenchmarkDotNet.csproj" />
  </ItemGroup>

</Project>

and program.cs without consuming type from BenchmarkDotNet.Diagnostics.Windows:

using System;
using System.Security.Cryptography;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

namespace ConsoleApp1
{
   
    public class Md5VsSha256
    {
        private const int N = 10000;
        private readonly byte[] data;

        private readonly SHA256 sha256 = SHA256.Create();
        private readonly MD5 md5 = MD5.Create();

        public Md5VsSha256()
        {
            data = new byte[N];
            new Random(42).NextBytes(data);
        }

        [Benchmark]
        public byte[] Sha256() => sha256.ComputeHash(data);

        [Benchmark]
        public byte[] Md5() => md5.ComputeHash(data);
    }
    class Program
    {
        static void Main(string[] args)
        {
            var switcher = BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);
        }
    }
}

After build my release folder looks like:
image

I was running test by:

dotnet ConsoleApp1.dll --filter *Md5* --profiler ETW

Before my changes I got exception. But of course I can try your path. I'll let you know.

@wojtpl2

This comment has been minimized.

Copy link
Collaborator

wojtpl2 commented Nov 15, 2018

I check your path, and I have to say that it works. DependencyContext doesn't load assambly. It only read *.deps.json file.

@eerhardt explained exactly why it works.:

DependencyContext allows you to get the information about a dependency, but it doesn't have assembly loading capabilities. You still use the normal Assembly.Load APIs to load the assembly.

The reason I suggested using DependencyContext is because I assumed the main thing you need/want here is you want to get the AssemblyVersion of the referenced BenchmarkDotNet.Diagnostics.Windows.dll, right? (That information can be found on this API).

If it works to just Assembly.Load(new AssemblyName(DiagnosticAssemblyName));, then you don't need the DependencyContext nor the bit about Assembly.GetEntryAssembly()?.GetReferencedAssemblies() at all. Just call Assembly.Load passing in the .dll name.

@wojtpl2

This comment has been minimized.

Copy link
Collaborator

wojtpl2 commented Nov 15, 2018

Without changes I got exception:

c:\Work\Test_955
λ dotnet run -c Release -- --filter * --profiler ETW
Error loading BenchmarkDotNet.Diagnostics.Windows.dll: FileNotFoundException - Could not load file or assembly 'c:\Work\Test_955\BenchmarkDotNet.Diagnostics.Windows.dll'. The system cannot find the file specified.
// Validating benchmarks:
Unable to resolve IProfiler diagnoser using dynamic assembly loading.
            Please make sure that you have installed the latest BenchmarkDotNet.Diagnostics.Windows package.
If you are using `dotnet build` you also need to consume one of its public types to make sure that MSBuild copies it to the output directory. The alternative is to use `<CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies>` in your project file.

After changes:

c:\Work\Test_955
dotnet run -c Release -- --filter * --profiler ETW
// Validating benchmarks:
// ***** BenchmarkRunner: Start   *****
@adamsitnik

This comment has been minimized.

Copy link
Member

adamsitnik commented Nov 15, 2018

@wojtpl2 Great!

btw I am an idiot. There is much simpler way of testing it: just create new project, install BDN packages and copy-paste the code for loading the dlls to the new app and run it.

@adamsitnik adamsitnik merged commit 376a97e into dotnet:master Nov 15, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@AndreyAkinshin AndreyAkinshin added this to the v0.11.3 milestone Nov 15, 2018

@wojtpl2 wojtpl2 deleted the wojtpl2:issue_955 branch Nov 15, 2018

@wojtpl2

This comment has been minimized.

Copy link
Collaborator

wojtpl2 commented Nov 15, 2018

You are not an idiot ;) Thanks to you I started my journey with open source.
btw. what task can I take? Maybe I can help you in #912? It is interesting me. Or I can do something else.

@eerhardt

This comment has been minimized.

Copy link
Member

eerhardt commented Nov 15, 2018

Thanks, @wojtpl2, for the great work here! Fixing this issue will help quite a few customers, especially ML.NET, where this issue was spawned from.

@adamsitnik

This comment has been minimized.

Copy link
Member

adamsitnik commented Nov 15, 2018

@wojtpl2 I just tested it myself and everything works fine. Once again thanks for help!

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