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

[WIP] System.Linq.Expressions.Interpreter.CallInstruction.CanCreateArbitraryDelegates should respect IsDynamicCodeSupported #86758

Closed
wants to merge 1 commit into from

Conversation

ivanpovazan
Copy link
Member

This PR is marked as draft as its sole purpose is to test potential issues with the change.

The work reflects what @MichalStrehovsky noted regarding adaptations of Linq.Expressions to be AOT compilation friendly: #69410 (comment) and the suggested way of enabling the mentioned codepath proposed by @rolfbjarne

@ivanpovazan ivanpovazan added NO-REVIEW Experimental/testing PR, do NOT review it area-System.Linq.Expressions labels May 25, 2023
@ivanpovazan ivanpovazan self-assigned this May 25, 2023
@ghost
Copy link

ghost commented May 25, 2023

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

Issue Details

This PR is marked as draft as its sole purpose is to test potential issues with the change.

The work reflects what @MichalStrehovsky noted regarding adaptations of Linq.Expressions to be AOT compilation friendly: #69410 (comment) and the suggested way of enabling the mentioned codepath proposed by @rolfbjarne

Author: ivanpovazan
Assignees: ivanpovazan
Labels:

NO-REVIEW, area-System.Linq.Expressions

Milestone: -

@ivanpovazan ivanpovazan changed the title [WIP] CallInstruction creation should respect IsDynamicCodeSupported [WIP] System.Linq.Expressions.Interpreter.CallInstruction creation should respect IsDynamicCodeSupported May 25, 2023
@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan
Copy link
Member Author

Rerun the pipelines once: #86971 gets merged in

@ivanpovazan
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan
Copy link
Member Author

The failures seem unrelated.

@MichalStrehovsky would it make sense to add a unit test for NativeAOT/MonoAOT which verifies that:
CanCreateArbitraryDelegates=false

Maybe something like:

var linqExprAssembly = typeof(System.Linq.Expressions.Expression).Assembly;
var callInstrType = linqExprAssembly?.GetType("System.Linq.Expressions.Interpreter.CallInstruction");
var canCreateArbitraryDelegatesGetter = callInstrType?.GetMethod("get_CanCreateArbitraryDelegates", BindingFlags.NonPublic | BindingFlags.Static);
Assert.False(canCreateArbitraryDelegatesGetter?.Invoke(null, null));

@MichalStrehovsky
Copy link
Member

The failures seem unrelated.

@MichalStrehovsky would it make sense to add a unit test for NativeAOT/MonoAOT which verifies that: CanCreateArbitraryDelegates=false

Maybe something like:

var linqExprAssembly = typeof(System.Linq.Expressions.Expression).Assembly;
var callInstrType = linqExprAssembly?.GetType("System.Linq.Expressions.Interpreter.CallInstruction");
var canCreateArbitraryDelegatesGetter = callInstrType?.GetMethod("get_CanCreateArbitraryDelegates", BindingFlags.NonPublic | BindingFlags.Static);
Assert.False(canCreateArbitraryDelegatesGetter?.Invoke(null, null));

I don't have an opinion. The value of this switch affects runtime throughput and size. We should be able to observe this in size/throughput testing.

@ivanpovazan ivanpovazan changed the title [WIP] System.Linq.Expressions.Interpreter.CallInstruction creation should respect IsDynamicCodeSupported [WIP] System.Linq.Expressions.Interpreter.CallInstruction.CanCreateArbitraryDelegates should respect IsDynamicCodeSupported Jun 22, 2023
@eerhardt
Copy link
Member

FYI - note the snags that I hit in this area in #81803 (comment).

@eerhardt
Copy link
Member

Closing in favor of #88539.

@eerhardt eerhardt closed this Jul 10, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2023
@ivanpovazan ivanpovazan deleted the improve-aot-linq-expr branch August 15, 2023 09:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Linq.Expressions NO-REVIEW Experimental/testing PR, do NOT review it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants