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

LambdaExpression.CanCompileToIL should respect IsDynamicCodeSupported #80759

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Jan 18, 2023

With the new feature switch added in #39806, calling LambdaExpression.Compile is throwing a PlatformNotSupportedException. Instead, LambdaExpression should respect IsDynamicCodeSupported and switch to using the interpreter when IsDynamicCodeSupported is false.

This causes issues when dotnet run on the new dotnet new api -aot template from ASP.NET.

fail: Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware[1]
      An unhandled exception has occurred while executing the request.
      System.PlatformNotSupportedException: Dynamic code generation is not supported on this platform.
         at System.Reflection.Emit.AssemblyBuilder.ThrowDynamicCodeNotSupported()
         at System.Reflection.Emit.DynamicMethod.Init(String name, MethodAttributes attributes, CallingConventions callingConvention, Type returnType, Type[] signature, Type owner, Module m, Boolean skipVisibility, Boolean transparentMethod)
         at System.Reflection.Emit.DynamicMethod..ctor(String name, Type returnType, Type[] parameterTypes, Boolean restrictedSkipVisibility)
         at System.Linq.Expressions.Compiler.LambdaCompiler..ctor(AnalyzedTree tree, LambdaExpression lambda)
         at System.Linq.Expressions.Compiler.LambdaCompiler.Compile(LambdaExpression lambda)
         at System.Linq.Expressions.Expression`1.Compile()
         at Microsoft.AspNetCore.Http.RequestDelegateFactory.HandleRequestBodyAndCompileRequestDelegate(Expression responseWritingMethodCall, RequestDelegateFactoryContext factoryContext)

Notes

  1. An alternative option here would be to revert the change to DynamicMethod..ctor that throws when IsDynamicCodeSupported == false.

cc @DamianEdwards

With the new feature switch added in dotnet#39806, calling LambdaExpression.Compile is throwing a PlatformNotSupportedException. Instead, LambdaExpression should respect IsDynamicCodeSupported and switch to using the interpreter when IsDynamicCodeSupported is false.
@ghost
Copy link

ghost commented Jan 18, 2023

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

With the new feature switch added in #39806, calling LambdaExpression.Compile is throwing a PlatformNotSupportedException. Instead, LambdaExpression should respect IsDynamicCodeSupported and switch to using the interpreter when IsDynamicCodeSupported is false.

This causes issues when dotnet run on the new dotnet new api -aot template from ASP.NET.

fail: Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware[1]
      An unhandled exception has occurred while executing the request.
      System.PlatformNotSupportedException: Dynamic code generation is not supported on this platform.
         at System.Reflection.Emit.AssemblyBuilder.ThrowDynamicCodeNotSupported()
         at System.Reflection.Emit.DynamicMethod.Init(String name, MethodAttributes attributes, CallingConventions callingConvention, Type returnType, Type[] signature, Type owner, Module m, Boolean skipVisibility, Boolean transparentMethod)
         at System.Reflection.Emit.DynamicMethod..ctor(String name, Type returnType, Type[] parameterTypes, Boolean restrictedSkipVisibility)
         at System.Linq.Expressions.Compiler.LambdaCompiler..ctor(AnalyzedTree tree, LambdaExpression lambda)
         at System.Linq.Expressions.Compiler.LambdaCompiler.Compile(LambdaExpression lambda)
         at System.Linq.Expressions.Expression`1.Compile()
         at Microsoft.AspNetCore.Http.RequestDelegateFactory.HandleRequestBodyAndCompileRequestDelegate(Expression responseWritingMethodCall, RequestDelegateFactoryContext factoryContext)

Adding new tests is currently in progress. Opening as draft to get CI and initial feedback

cc @DamianEdwards

Author: eerhardt
Assignees: -
Labels:

area-System.Linq.Expressions

Milestone: -

@MichalPetryka
Copy link
Contributor

Would RuntimeFeature.IsDynamicCodeCompiled be a better check here?

@eerhardt
Copy link
Member Author

Would RuntimeFeature.IsDynamicCodeCompiled be a better check here?

No. Just because the generated code isn’t compiled (i.e. JIT’d), it doesn’t mean we should use the Linq.Expressions interpreter. The Mono Interpreter can probably do a better job at executing the generated IL than the Linq.Expressions interpreter can.

Note that the Mono interpreter is currently the only environment where these values will be different that affects this code. This is mainly for WASM. On iOS, this value is already false.

See #38693 (comment) for an example of a case where generating IL and then interpreting it is preferred.

@@ -154,7 +154,7 @@ public Delegate Compile()
/// <returns>A delegate containing the compiled version of the lambda.</returns>
public Delegate Compile(bool preferInterpretation)
{
if (CanCompileToIL && CanInterpret && preferInterpretation)
if (CanInterpret && preferInterpretation)
Copy link
Member

@MichalStrehovsky MichalStrehovsky Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if this impacts size with NativeAOT?

The dead branch elimination in NativeAOT only considers things that have feature switches. It doesn't attempt to inline things.

I think this is changing things from if (false && CanInterpret && preferInterpretation) to if (CanInterpret && preferInterpretation) (no longer a known constant).

Could be fixed by creating a substitution rule for CanInterpret as well.

(Either that or beefing up the dead branch elimination in NativeAOT. The dead branch elimination in NativeAOT is simpler than in linker because: 1. linker's one is super scary and I don't want to have to deal with the bugs in it that I would get by porting it, and 2. longer term, we should have RyuJIT do the dead branch elimination. RyuJIT is much better at it than anything we would write specifically for trimming.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe this is fine now that I look at it for longer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 reasons why I'm proposing this change.

  1. Logically, the condition doesn't make sense. Why would CanCompileToIL need to be true in order for preferInterpretation be respected?
  2. Looking at what it functionally does:
    public Delegate Compile()
    {
    if (CanCompileToIL)
    {
    return Compiler.LambdaCompiler.Compile(this);
    }
    else
    {
    Debug.Assert(CanInterpret);
    return new Interpreter.LightCompiler().CompileTop(this).CreateDelegate();
    }
    }
    /// <summary>
    /// Produces a delegate that represents the lambda expression.
    /// </summary>
    /// <param name="preferInterpretation">A <see cref="bool"/> that indicates if the expression should be compiled to an interpreted form, if available.</param>
    /// <returns>A delegate containing the compiled version of the lambda.</returns>
    public Delegate Compile(bool preferInterpretation)
    {
    if (CanCompileToIL && CanInterpret && preferInterpretation)
    {
    return new Interpreter.LightCompiler().CompileTop(this).CreateDelegate();
    }
    return Compile();
    }

    If CanCompileToIL == false, the same code (return new Interpreter.LightCompiler().CompileTop(this).CreateDelegate();) is going to be executed regardless.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it makes sense to do. I'm responsible for introducing this in #61952.

That pull request was a mechanical replacement of #defines. The code originally looked like:

        public Delegate Compile(bool preferInterpretation)
        {
#if FEATURE_COMPILE && FEATURE_INTERPRET
            if (preferInterpretation)
            {
                return new Interpreter.LightCompiler().CompileTop(this).CreateDelegate();
            }
#endif
            return Compile();
        }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a second look, the original code doesn't make that much more sense either...

@eerhardt eerhardt marked this pull request as ready for review January 18, 2023 18:29
@eerhardt eerhardt merged commit 29075ba into dotnet:main Jan 19, 2023
@eerhardt eerhardt deleted the FixLinqExpressionsDynamicCode branch January 19, 2023 15:10
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
…dotnet#80759)

* LambdaExpression.CanCompileToIL should respect IsDynamicCodeSupported

With the new feature switch added in dotnet#39806, calling LambdaExpression.Compile is throwing a PlatformNotSupportedException. Instead, LambdaExpression should respect IsDynamicCodeSupported and switch to using the interpreter when IsDynamicCodeSupported is false.

* Add tests
@ghost ghost locked as resolved and limited conversation to collaborators Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants