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

[API Proposal]: AssemblyLoadContext.TryLoadFromAssemblyName #87185

Open
jaredpar opened this issue Jun 6, 2023 · 14 comments
Open

[API Proposal]: AssemblyLoadContext.TryLoadFromAssemblyName #87185

jaredpar opened this issue Jun 6, 2023 · 14 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-AssemblyLoader-coreclr
Milestone

Comments

@jaredpar
Copy link
Member

jaredpar commented Jun 6, 2023

Background and motivation

The C# compiler uses AssemblyLoadContext to isolate and manage analyzers and generators that plug into the compiler. Analyzers are passed to the compiler as a series of /analyzer:path/to/analyzer1.dll arguments. The compiler effectively groups these arguments by directory and creates an AssemblyLoadContext per directory.

The problem is that due to the nature of build, NuPkg authoring and analyzer detection, the compiler will end up getting passed DLLs that are not a part of the analyzer but instead part of the compiler. For example it's not uncommon to see /analyzer:System.Collections.Immutable.dll or /analyzer:System.Runtime.CompilerServices.Unsafe.dll to be passed as arguments. This is problematic because these DLLs contain exchange types. The compiler owns these DLLs and their copy must be used in both the compiler and analyzer for proper functioning. Loading the analyzer copy will lead to API mismatches later on that break the compilation process.

Today the only way to determine if the compiler owns the DLL is to first attempt to load the DLL into the compiler AssemblyLoadContext via LoadFromAssemblyName and if that succeeds use that DLL, otherwise load into the analyzer AssemblyLoadContext. That approach works great but has the downside that it introduces first chance FileNotFoundException instances because LoadFromAssemblyName throws on failure hence our core load path is as follows:

protected override Assembly? Load(AssemblyName assemblyName)
{
    var simpleName = assemblyName.Name!;
    try
    {
        if (_compilerLoadContext.LoadFromAssemblyName(assemblyName) is { } compilerAssembly)
        {
            return compilerAssembly;
        }
    }
    catch
    {
        // Expected to happen when the assembly cannot be resolved in the compiler / host
        // AssemblyLoadContext.
    }

    // Proceed with loading in the this analyzer AssemblyLoadContext

The compiler is hosted in a number of applications including Visual Studio. The Visual Studio team keeps tabs on first chance exceptions in core scenarios because it can contribute negatively to startup performance. This means the compiler and Visual Studio are at a tension point when it comes to one of our core scenarios. Every time we add or change analyzers / generators it introduces new first chance exceptions into the product, flags our insertions and requires discussion to resolve.

The motivation here is to have an API that does not throw here. Asking an AssemblyLoadContext to load an assembly and having it fail is not necessarily an exceptional item.

Note: happy to elaborate on why these unnecessary DLLs get passed by that is a problem inherent to both our ecosystem as well as other similar .NET plugin situations. Solving that is likely not realistic which is why the request for an API solution (it also seems reasonable by itself).

API Proposal

namespace System.Runtime.Loader

public class AssemblyLoadContext
{
    public bool TryLoadFromAssemblyName(AssemblyName assemblyName, [NotNullWhen(true)] out Assembly? assembly)
}

This API would function exactly as LoadFromAssemblyName does today except that it uses a bool to express failure instead of an exception.

API Usage

Given that code paths could change to the following

protected override Assembly? Load(AssemblyName assemblyName)
{
    var simpleName = assemblyName.Name!;
    if (_compilerLoadContext.TryLoadFromAssemblyName(assemblyName, out var compilerAssembly))
    {
        return compilerAssembly;
    }

    // Proceed with loading in the this analyzer AssemblyLoadContext

Alternative Designs

A potential alternative design is to have an oveload of LoadFromAssemblyName which has a throwOnError parameter similar to Type.GetType.

public class AssemblyLoadContext
{
    public Assembly? TryLoadFromAssemblyName(AssemblyName assemblyName, bool throwOnError)
}

That is undesirable for the following reasons:

  1. The API does not work well with nullable reference types.
  2. The Try pattern generally the standard approach for this style of method.

Risks

None that I can think of. In discussing with @elinor-fung and @davidwrighton they felt this was a reasonable approach to solving the problem.

@jaredpar jaredpar added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 6, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 6, 2023
@ghost
Copy link

ghost commented Jun 6, 2023

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

The C# compiler uses AssemblyLoadContext to isolate and manage analyzers and generators that plug into the compiler. Analyzers are passed to the compiler as a series of /analyzer:path/to/analyzer1.dll arguments. The compiler effectively groups these arguments by directory and creates an AssemblyLoadContext per directory.

The problem is that due to the nature of build, NuPkg authoring and analyzer detection, the compiler will end up getting passed DLLs that are not a part of the analyzer but instead part of the compiler. For example it's not uncommon to see /analyzer:System.Collections.Immutable.dll or /analyzer:System.Runtime.CompilerServices.Unsafe.dll to be passed as arguments. This is problematic because these DLLs contain exchange types. The compiler owns these DLLs and their copy must be used in both the compiler and analyzer for proper functioning. Loading the analyzer copy will lead to API mismatches later on that break the compilation process.

Today the only way to determine if the compiler owns the DLL is to first attempt to load the DLL into the compiler AssemblyLoadContext via LoadFromAssemblyName and if that succeeds use that DLL, otherwise load into the analyzer AssemblyLoadContext. That approach works great but has the downside that it introduces first chance exceptions because LoadFromAssemblyName throws on failure hence our core load path is as follows:

protected override Assembly? Load(AssemblyName assemblyName)
{
    var simpleName = assemblyName.Name!;
    try
    {
        if (_compilerLoadContext.LoadFromAssemblyName(assemblyName) is { } compilerAssembly)
        {
            return compilerAssembly;
        }
    }
    catch
    {
        // Expected to happen when the assembly cannot be resolved in the compiler / host
        // AssemblyLoadContext.
    }

    // Proceed with loading in the this analyzer AssemblyLoadContext

The compiler is hosted in a number of applications including Visual Studio. The Visual Studio team keeps tabs on first chance exceptions in core scenarios because it can contribute negatively to startup performance. This means the compiler and Visual Studio are at a tension point when it comes to one of our core scenarios. Every time we add or change analyzers / generators it introduces new first chance exceptions into the product, flags our insertions and requires discussion to resolve.

The motivation here is to have an API that does not throw here. Asking an AssemblyLoadContext to load an assembly and having it fail is not necessarily an exceptional item.

Note: happy to elaborate on why these unnecessary DLLs get passed by that is a problem inherent to both our ecosystem as well as other similar .NET plugin situations. Solving that is likely not realistic which is why the request for an API solution (it also seems reasonable by itself).

API Proposal

namespace System.Runtime.Loader

public class AssemblyLoadContext
{
    public bool TryLoadFromAssemblyName(AssemblyName assemblyName, [NotNullWhen(true)] out Assembly? assembly)
}

This API would function exactly as LoadFromAssemblyName does today except that it uses a bool to express failure instead of an exception.

API Usage

Given that code paths could change to the following

protected override Assembly? Load(AssemblyName assemblyName)
{
    var simpleName = assemblyName.Name!;
    if (_compilerLoadContext.TryLoadFromAssemblyName(assemblyName, out var compilerAssembly))
    {
        return compilerAssembly;
    }

    // Proceed with loading in the this analyzer AssemblyLoadContext

Alternative Designs

A potential alternative design is to have an oveload of LoadFromAssemblyName which has a throwOnError parameter similar to Type.GetType.

public class AssemblyLoadContext
{
    public Assembly? TryLoadFromAssemblyName(AssemblyName assemblyName, bool throwOnError)
}

That is undesirable for the following reasons:

  1. The API does not work well with nullable reference types.
  2. The Try pattern generally the standard approach for this style of method.

Risks

None that I can think of. In discussing with @elinor-fung and @davidwrighton they felt this was a reasonable approach to solving the problem.

Author: jaredpar
Assignees: -
Labels:

api-suggestion, area-AssemblyLoader-coreclr

Milestone: -

@jkotas
Copy link
Member

jkotas commented Jun 7, 2023

Similar API proposal #21785, with similar motivation, has been rejected by API review a few years back.

@vitek-karas
Copy link
Member

The way I view this request is slightly different from #21785. The core question which this wants to answer is: "Can this ALC resolve this assembly name?". It doesn't necessarily need to actually resolve it (it can call the throwing method to do so). And in a way it may not need to be 100% correct (for example the context may know how to resolve it, but the file is missing on disk and the load eventually fails, or something like that).

Another way to look at this is that each ALC has a set of assembly names it knows how to resolve. But it never materializes the set - that's done lazily. The core of the question is "is my assembly name in that set?" - but we can't answer that with a lookup, it has to run the resolution algorithm.

Ultimately this is definitely doable, the problematic areas I can think of right now:

  • Defining the desired functionality - if we want to just expose the "test" or we want the real "load but don't throw" behavior.
  • What should happen if the resolution runs into problems - we're running potentially lot of custom code during the resolution and it can fail in many different ways - do we propagate the failure as exception even through the Try call, or do we swallow it and turn it into a simple false? (The answer is probably "it depends", so it's a bit complicated).
  • Do we try to make this a true "Try" behavior - meaning that calling the method and it returning false doesn't modify any global state (getting a false is a non-side-effecting operation) - that is normally what the Try methods behave like, but doing this in the assembly resolver would be VERY difficult.

@vitek-karas
Copy link
Member

/cc @elinor-fung

@jkotas
Copy link
Member

jkotas commented Jun 7, 2023

Defining the desired functionality - if we want to just expose the "test" or we want the real "load but don't throw" behavior.

I do not think we would want to be creative with introducing new partial assembly loading models. The behavior of the API should 100% match the following implementation, except that it will try to avoid throwing and catching FileLoadExceptions internally where possible.

bool TryLoadFromAssemblyName(AssemblyName assemblyName, [NotNullWhen(true)] out Assembly? assembly)
{
    try
    {
        assembly = LoadFromAssemblyName(assemblyName(assemblyName);
        return true;
    }
    catch (FileLoadException e)
    {
        assembly = null;
        return false;
    }
}

@jkotas
Copy link
Member

jkotas commented Jun 7, 2023

The Visual Studio team keeps tabs on first chance exceptions in core scenarios because it can contribute negatively to startup performance.

Note that we have work underway to make throwing and catching exceptions a lot cheaper. It would be useful to measure the cost of throwing and catching exception (after the perf improvements) and compare it with the total cost of analyzer assembly load in Roslyn scenarios. It is quite possible that this API would not actually result in any meaningful perf improvement.

@agocke agocke added this to AppModel Jul 10, 2023
@agocke agocke added this to the 9.0.0 milestone Aug 9, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 9, 2023
@stephentoub stephentoub modified the milestones: 9.0.0, 10.0.0 Jul 22, 2024
@CyrusNajmabadi
Copy link
Member

Note: this issue is currently impeding getting in SG reloading in VS :)

Specifically, all the exceptions thrown now by this helper are triggering the gates in speedometer: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/581245

To get an idea:

Image

Would love to get a helper here that allows us to have this functionality without exceptions. Note that exceptions are also painful for other reasons (like debugging) as they make it seem as if there's a problem, when really there isn't. Would love to see this prioritized.

@jaredpar
Copy link
Member Author

jaredpar commented Sep 30, 2024

Note that exceptions are also painful for other reasons (like debugging) as they make it seem as if there's a problem, when really there isn't

It also just makes debugging harder. Every time I debug the compiler know I know that I'm going to have to step through 5-6 exceptions that I don't actually care about. They are noise that I cannot disable if I'm digging into loading bugs in the compiler. It's a persistent negative part of our developer experience.

@ryanmolden
Copy link

ryanmolden commented Sep 30, 2024

Note that exceptions are also painful for other reasons (like debugging) as they make it seem as if there's a problem, when really there isn't

It also just makes debugging harder. Every time I debug the compiler know I know that I'm going to have to step through 5-6 exceptions that I don't actually care about. They are noise that I cannot disable if I'm digging into loading bugs in the compiler. It's a persistent negative part of our developer experience.

I would like to +100 this. As a developer that routinely debugs VS we throw way too many exceptions for normal/expected things. As cheap as they may be for an end-user (i.e. the runtime, non-debugger cost of throwing/catching exceptions) they will always be expensive under a debugger as the debugger has to field the FCE notification, pause all threads in the running app, decide if the user has that exception marked for break on FCE, and then do the appropriate thing (which may just be 'continue execution', i.e. a rather expensive NOP).

One could certainly argue part of the cost here is in the debugger reaction to the FCE, and I agree, but one could also argue that not doing something (i.e. throwing an exception and someone having to deal with it) will always be cheaper than doing that same thing, as far as runtime cost goes.

Adding cost here via adding more and more 'ignorable' exceptions significantly impacts the dev inner loop and thus makes us less productive in actually developing Visual Studio as it adds friction to a workflow we engage in many, many times a day. It also makes the whole 'exceptions are exceptional' argument a bit less convincing since in this case it seems like these are NOT actually exceptional/unexpected situations.

A particularly egregious example of this is triage dump debugging wherein Roslyn seems to trigger many thousand exceptions when you start debugging and it literally means for me, that being under a debugger in that scenario takes (no exaggeration) multiple minutes to get debugging started, whereas not under a debugger it takes seconds.

@CyrusNajmabadi
Copy link
Member

A particularly egregious example of this is triage dump debugging wherein Roslyn seems to trigger many thousand exceptions

Are these cancellation exceptions, or something else. If something else, let us know what they are so we can resolve. If they're cancellation, @jcouv was working on changing compiler code-gen i believe so that async-cancellation doesn't involve N rethrows of hte CT across each await in an async chain.

This comment has been minimized.

@jkotas

This comment has been minimized.

@bartonjs

This comment has been minimized.

@bartonjs bartonjs added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-AssemblyLoader-coreclr and removed area-System.Reflection labels Jan 10, 2025

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-AssemblyLoader-coreclr
Projects
Status: No status
Development

No branches or pull requests

8 participants