Start building a universal System.Linq.Expressions library #61952

MichalStrehovsky · 2021-11-23T04:30:49Z

System.Linq.Expressions currently offers multiple build time definitions to build different flavors of the library:

FEATURE_COMPILE (defined everywhere except iOS/tvOS/Catalyst, and NativeAOT) - enables Reflection.Emit-based expression tree compiler.
FEATURE_INTERPRET (always defined and not actually possible to build without) - enables the expression interpreter
FEATURE_DLG_INVOKE (defined everywhere except NativeAOT, but we likely need to be able to run without it on iOS too because there's uninvestigated bugs around it ActiveIssue'd in Make LambdaCompiler prefer interpretation to compilation on iOS #54970) - in the interpreter, use a delegate to invoke CallInstructions instead of MethodInfo.Invoke. The delegate might have to be Reflection.Emitted if it's not supportable with Func/Action (that's the uninvestigated iOS/tvOS/Catalyst bug).

For #61231, we need to be able to build a single System.Linq.Expression library that can switch between these build-time configurations at publish time since we don't want to build a separate S.L.Expressions library for NativeAOT. There are advantages in having this setup for non-NativeAOT scenarios too. This pull request accomplishes that by mechanically changing the #defines into feature switches.

The feature switch is placed in the last proposed location for #17973. I expect we'll want such API to be public at some point; now that Xamarin and NativeAOT use this formerly .NET Native-only thing the API request became relevant again.

This pull request is focused on the mechanical replacement of #defines with feature switches and it's already a lot bigger than I'm comfortable with.

There's some obvious "!FEATURE_COMPILE means this is .NET Native with everything what that meant" that I did not touch because this is meant to be a mechanical change. Some cleanup will be needed at some point. Right now this just mostly means we're running fewer tests than we could.

Validation:

Verified that we're still running the same number of tests with CoreCLR as we previously were and they're all passing.
Verified we're getting mostly the same size of the S.L.Expressions library on iOS (433 kB grew to ~~437~~ 436 kB, the diffs are expected).
Verified things work on the NativeAOT side as well.

Cc @jkotas @marek-safar @MaximLipnin @steveisok @akoeplinger

System.Linq.Expressions currently offers multiple build time definitions to build different flavors of the library: * `FEATURE_COMPILE` (defined everywhere except iOS/tvOS/Catalyst, and NativeAOT) - enables Reflection.Emit-based expression tree compiler. * `FEATURE_INTERPRET` (always defined and not actually possible to build without) - enables the expression interpreter * `FEATURE_DLG_INVOKE` (defined everywhere except NativeAOT, but we likely need to be able to run without it on iOS too because there's uninvestigated bugs around it ActiveIssue'd in dotnet#54970) - in the interpreter, use a delegate to invoke `CallInstructions` instead of `MethodInfo.Invoke`. The delegate might have to be Reflection.Emitted if it's not supportable with `Func`/`Action` (that's the uninvestigated iOS/tvOS/Catalyst bug). For dotnet#61231, we need to be able to build a single System.Linq.Expression library that can switch between these build-time configurations _at publish time_ since we don't want to build a separate S.L.Expressions library for NativeAOT. There are advantages in having this setup for non-NativeAOT scenarios too. This pull request accomplishes that by mechanically changing the `#define`s into feature switches. The feature switch is placed in the last proposed location for dotnet#17973. I expect we'll want such API to be public at some point; now that Xamarin and NativeAOT use this formerly .NET Native-only thing the API request became relevant again. This pull request is focused on the mechanical replacement of `#defines` with feature switches and it's already a lot bigger than I'm comfortable with. There's some obvious "`!FEATURE_COMPILE` means this is .NET Native with everything what that meant" that I did not touch because this is meant to be a mechanical change. Some cleanup will be needed at some point. Right now this just mostly means we're running fewer tests than we could. Validation: * Verified that we're still running the same number of tests with CoreCLR as we previously were and they're all passing. * Verified we're getting mostly the same size of the S.L.Expressions library on iOS (433 kB grew to 437 kB, the diffs are expected). * Verified things work on the NativeAOT side as well.

ghost · 2021-11-23T04:31:03Z

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

Issue Details

System.Linq.Expressions currently offers multiple build time definitions to build different flavors of the library:

FEATURE_COMPILE (defined everywhere except iOS/tvOS/Catalyst, and NativeAOT) - enables Reflection.Emit-based expression tree compiler.
FEATURE_INTERPRET (always defined and not actually possible to build without) - enables the expression interpreter
FEATURE_DLG_INVOKE (defined everywhere except NativeAOT, but we likely need to be able to run without it on iOS too because there's uninvestigated bugs around it ActiveIssue'd in Make LambdaCompiler prefer interpretation to compilation on iOS #54970) - in the interpreter, use a delegate to invoke CallInstructions instead of MethodInfo.Invoke. The delegate might have to be Reflection.Emitted if it's not supportable with Func/Action (that's the uninvestigated iOS/tvOS/Catalyst bug).

For #61231, we need to be able to build a single System.Linq.Expression library that can switch between these build-time configurations at publish time since we don't want to build a separate S.L.Expressions library for NativeAOT. There are advantages in having this setup for non-NativeAOT scenarios too. This pull request accomplishes that by mechanically changing the #defines into feature switches.

The feature switch is placed in the last proposed location for #17973. I expect we'll want such API to be public at some point; now that Xamarin and NativeAOT use this formerly .NET Native-only thing the API request became relevant again.

This pull request is focused on the mechanical replacement of #defines with feature switches and it's already a lot bigger than I'm comfortable with.

There's some obvious "!FEATURE_COMPILE means this is .NET Native with everything what that meant" that I did not touch because this is meant to be a mechanical change. Some cleanup will be needed at some point. Right now this just mostly means we're running fewer tests than we could.

Validation:

Verified that we're still running the same number of tests with CoreCLR as we previously were and they're all passing.
Verified we're getting mostly the same size of the S.L.Expressions library on iOS (433 kB grew to 437 kB, the diffs are expected).
Verified things work on the NativeAOT side as well.

Cc @jkotas @marek-safar @MaximLipnin @steveisok @akoeplinger

Author:	MichalStrehovsky
Assignees:	-
Labels:	`area-System.Linq.Expressions`
Milestone:	-

src/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/DelegateHelpers.cs

MaximLipnin · 2021-11-23T11:23:47Z

/azp run runtime-manual

azure-pipelines · 2021-11-23T11:24:03Z

Azure Pipelines successfully started running 1 pipeline(s).

marek-safar · 2021-11-23T12:26:17Z

src/libraries/System.Linq.Expressions/src/ILLink/ILLink.Substitutions.xml

+    <type fullname="System.Dynamic.Utils.DelegateHelpers">
+      <method signature="System.Boolean get_CanEmitObjectArrayDelegate()" feature="System.Linq.Expressions.CanEmitObjectArrayDelegate" featurevalue="false" body="stub" value="false" />
+    </type>
+    <type fullname="System.Linq.Expressions.Interpreter.CallInstruction">
+      <method signature="System.Boolean get_CanCreateArbitraryDelegates()" feature="System.Linq.Expressions.CanCreateArbitraryDelegates" featurevalue="false" body="stub" value="false" />
+    </type>


Shouldn't these be RuntimeFeature(s) instead?

Property living in Expressions was the last direction #17973 (comment) discussed. I'm just following that, given the API is unreviewed.

get_CanEmitObjectArrayDelegate and get_CanCreateArbitraryDelegates probably won't be APIs ever. CanCompileToIL might.

I don't understand exactly what they try to do but it seems to me they react to runtime limitations, right? Should not this be then call to runtime to get the actual value with a feature switch on top of it?

get_CanEmitObjectArrayDelegate and get_CanCreateArbitraryDelegates could probably be replaced with RuntimeFeature.IsDynamicCodeSupported eventually, but it would be a behavior change and this pull request didn't intend to do behavior changes, just replace #defines with something that can be overriden.

Replacing get_CanCreateArbitraryDelegates with IsDynamicCodeSupported would make the iDevices version take the codepaths that correspond to FEATURE_DLG_INVOKE not being defined in the csproj and that's not how iDevices build was set up in #54970. If you would like me to make that change, I can do it, but I cannot investigate any possible fallouts from it because I don't work on or own iDevices. An observable difference will be slower perf when invoking method call instructions in the interpreter. An upside will be that some of the tests that were blocked in #54970 will work. Ideally all of this (configuring the library so that it properly works, and revisiting tests blocked in #54970) should happen in subsequent pull requests. I definitely plan to revisit the blocked tests.

get_CanEmitObjectArrayDelegate cannot be replaced with RuntimeFeature.IsDynamicCodeSupported at this moment because it requires a runtime API that is not implemented on Mono and would therefore break iDevices (it's an API to create a strongly typed Delegate that boxes all it's arguments, puts them into an array, dispatches to a Func<object?[], object?> delegate to do the actual work, and unboxes the result - doing all of that without runtime codegen).

marek-safar · 2021-11-23T12:29:49Z

src/libraries/System.Linq.Expressions/src/ILLink/ILLink.Substitutions.xml

+<linker>
+  <assembly fullname="System.Linq.Expressions">
+    <type fullname="System.Linq.Expressions.LambdaExpression">
+      <method signature="System.Boolean get_CanCompileToIL()" feature="System.Linq.Expressions.CanCompileToIL" featurevalue="false" body="stub" value="false" />


Why new feature switch for code compilation mode? Also the defaults don't look correct for existing configurations (e.g. published trimmed console app).

#17973 (comment) has reasoning for why this needs to be different from RuntimeFeature.IsDynamicCodeSupported.

Also the defaults don't look correct for existing configurations (e.g. published trimmed console app).

Could you clarify?

has reasoning for why this needs to be different from

I don't see the reasoning in the comment and this mode is not new in .NET7. In .NET6 we use the same options to detect NativeAOT like setup where IsDynamicCodeSupported indicates that code generation is possible with IsDynamicCodeCompiled specifying how it such generated code processed.

Could you clarify?

What does happen today when you publish trimmed console for desktops. I guess it has "CanCompileToIL" set to true and here we are setting it to false for everything.

I don't see the reasoning in the comment and this mode is not new in .NET7. In .NET6 we use the same options to detect NativeAOT like setup where IsDynamicCodeSupported indicates that code generation is possible with IsDynamicCodeCompiled specifying how it such generated code processed.

We could make this so that the body of CanCompileToIL is implemented as RuntimeFeature.IsDynamicCodeSupported, but I'm a believer in separating behavioral changes from refactoring changes and this would be a behavioral change. It can happen in subsequent pull request as a thing that can be discussed separately from this refactoring.

What does happen today when you publish trimmed console for desktops. I guess it has "CanCompileToIL" set to true and here we are setting it to false for everything.

The substitution is set as feature="X" featurevalue="false" body="stub" value="false" - if I'm reading IL Linker sources right, this substitution will be ignored unless someone sets feature X to false. Is that correct reading? I wasn't able to build intuition behind what featurevalue=X featuredefault=Y means and always defer to reading linker source code. If my interpretation is correct, this will do the right thing for trimmed apps (IL compiler will be kept).

src/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/DelegateHelpers.cs

MichalStrehovsky · 2021-11-24T05:30:09Z

@MaximLipnin I can't make much sense from the runtime-manual run because it seems to have many unrelated failures. Could you please see if there's anything needing my attention?

MaximLipnin · 2021-11-24T06:58:04Z

@MaximLipnin I can't make much sense from the runtime-manual run because it seems to have many unrelated failures. Could you please see if there's anything needing my attention?

@MichalStrehovsky JFYI - By default, only System.Runtime test suite executes for mobile platforms. The runtime-manual lanes run the whole set of libraries test suites (all the unrelated issue are going to be fixed soon). One can run it on every significant push (in case it can affect mobile targets). There is also a rolling build which will catch anything anyways.

agocke · 2021-11-30T02:31:40Z

@cston @jaredpar Could you take a look at this PR to change compile-time #defines into feature flags? This should allow us to create one copy of System.Linq.Expressions with both reflection and interpreter modes, where the behavior can be controlled at publish time. It shouldn't have any functional change for the default project.

agocke · 2021-11-30T02:33:11Z

@vitek-karas could you double check the runtime feature flag stuff here?

vitek-karas

The feature switch setup looks good. Assuming we discussed which feature switches we want already (I don't know enough to comment on that).

cston · 2021-11-30T20:35:56Z

src/libraries/System.Linq.Expressions/tests/InterpreterTests.cs

@@ -15,7 +11,9 @@ public static class InterpreterTests
    {
        private static readonly PropertyInfo s_debugView = typeof(LightLambda).GetPropertyAssert("DebugView");

-        [Fact]
+        // FEATURE_COMPILE is not directly required,


FEATURE_COMPILE

Should the comment say LambdaExpression.CanCompileToIL instead, here and below, since there are no longer references to FEATURE_COMPILE?

Fixed those all.

Once this is merged, I'll do a test pass on CoreCLR with the IL Compiler disabled and see whether these comments are correct. If the comment is correct, I'll change the condition from "is IL compiler available" to "is Reflection.Emit available" and delete the comment, because that's what we're after.

cston · 2021-11-30T20:40:10Z

src/libraries/System.Linq.Expressions/tests/Invoke/InvocationTests.cs

-#if FEATURE_COMPILE // When we don't have FEATURE_COMPILE we don't have the Reflection.Emit used in the tests.
-
-        [Theory, ClassData(typeof(CompilationTypes))]
+        // When we don't have FEATURE_COMPILE we don't have the Reflection.Emit used in the tests.


FEATURE_COMPILE

LambdaExpression.CanCompileToIL here and below?

cston · 2021-11-30T20:40:42Z

src/libraries/System.Linq.Expressions/tests/Lambda/LambdaTests.cs

@@ -268,9 +268,11 @@ public void ImplicitlyTyped()
                double, double, double, double,
                bool>), exp.Type);

-#if FEATURE_COMPILE
            // From this point on, the tests require FEATURE_COMPILE (RefEmit) support as SLE needs to create delegate types on the fly.


FEATURE_COMPILE

LambdaExpression.CanCompileToIL?

dotnet-issue-labeler bot added the area-System.Linq.Expressions label Nov 23, 2021

MichalStrehovsky commented Nov 23, 2021

View reviewed changes

src/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/DelegateHelpers.cs Outdated Show resolved Hide resolved

MichalStrehovsky mentioned this pull request Nov 23, 2021

Warning getting generated for lightup patterns dotnet/linker#2392

Open

Work around linker bug

c836ba6

MichalStrehovsky mentioned this pull request Nov 23, 2021

Skip Linq tests, which are not supported in interpreter only build (without FEATURE_COMPILE) #61914

Closed

marek-safar reviewed Nov 23, 2021

View reviewed changes

Disable ipconstprop

050cae3

vitek-karas approved these changes Nov 30, 2021

View reviewed changes

cston reviewed Nov 30, 2021

View reviewed changes

cston approved these changes Dec 1, 2021

View reviewed changes

Review feedback

d2ba72d

MichalStrehovsky merged commit 09cd9c2 into dotnet:main Dec 1, 2021

MichalStrehovsky deleted the expressions branch December 1, 2021 05:01

joshpeterson mentioned this pull request Dec 1, 2021

bot upstream main merge 2021 12 01 Unity-Technologies/runtime#12

Closed

dotnet locked as resolved and limited conversation to collaborators Dec 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start building a universal System.Linq.Expressions library #61952

Start building a universal System.Linq.Expressions library #61952

MichalStrehovsky commented Nov 23, 2021 •

edited

ghost commented Nov 23, 2021

MaximLipnin commented Nov 23, 2021

azure-pipelines bot commented Nov 23, 2021

marek-safar Nov 23, 2021

MichalStrehovsky Nov 23, 2021

marek-safar Nov 24, 2021

MichalStrehovsky Nov 25, 2021

marek-safar Nov 23, 2021

MichalStrehovsky Nov 23, 2021

marek-safar Nov 24, 2021

MichalStrehovsky Nov 25, 2021

MichalStrehovsky commented Nov 24, 2021

MaximLipnin commented Nov 24, 2021

agocke commented Nov 30, 2021

agocke commented Nov 30, 2021

vitek-karas left a comment

cston Nov 30, 2021

MichalStrehovsky Dec 1, 2021

cston Nov 30, 2021

cston Nov 30, 2021

Start building a universal System.Linq.Expressions library #61952

Start building a universal System.Linq.Expressions library #61952

Conversation

MichalStrehovsky commented Nov 23, 2021 • edited

ghost commented Nov 23, 2021

MaximLipnin commented Nov 23, 2021

azure-pipelines bot commented Nov 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky commented Nov 24, 2021

MaximLipnin commented Nov 24, 2021

agocke commented Nov 30, 2021

agocke commented Nov 30, 2021

vitek-karas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky commented Nov 23, 2021 •

edited