Skip to content

Basic support of InProcessNoEmitRunner for NativeAOT. #2702

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

Merged
merged 3 commits into from
Mar 2, 2025

Conversation

eliphatfs
Copy link
Contributor

Supports benchmark methods with void or Task as return type.

@eliphatfs
Copy link
Contributor Author

See #2701.

@timcassell
Copy link
Collaborator

Does it also fix int or Task<int> returns?

@eliphatfs
Copy link
Contributor Author

eliphatfs commented Mar 1, 2025

It doesn't fix int or Task<int> returns.
I don't think of a general way for supporting arbitrary return types (user-defined classes/structs etc.).
For basic types (bool, byte, int, uint, long, etc.) though we may be able to enumerate them and either call them explicitly as special cases or use in a dummy helper class and declare dynamic dependency to that helper class. Would you like me to do that?

@eliphatfs
Copy link
Contributor Author

For the general return type to work we may expose a generic helper class for users to declare themselves the dependencies with. Not zero-instrumentation, but may be acceptable.

@timcassell
Copy link
Collaborator

For the general return type to work we may expose a generic helper class for users to declare themselves the dependencies with. Not zero-instrumentation, but may be acceptable.

That sounds like a better option. I don't want to special case a bunch of types.

@timcassell
Copy link
Collaborator

On second thought, I don't really want to add complex APIs just for this toolchain. The ultimate solution will be #1770.

Tests are failing, please check.

Copy link
Collaborator

@timcassell timcassell left a comment

Choose a reason for hiding this comment

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

Thanks @eliphatfs

@timcassell timcassell linked an issue Mar 2, 2025 that may be closed by this pull request
@timcassell timcassell merged commit 6ce9795 into dotnet:master Mar 2, 2025
8 checks passed
@timcassell timcassell added this to the v0.14.1 milestone Mar 2, 2025
@MichalStrehovsky
Copy link
Member

If you were okay harcoding assembly name into string inProcessRunnableTypeName = $"{typeof(InProcessNoEmitRunner).FullName}+{nameof(Runnable)}"; (i.e. add a comma and put the assembly name after the comma) and use Type.GetType(inProcessRunnableTypeName) instead of the typeof(InProcessNoEmitRunner).GetTypeInfo().Assembly.GetType(inProcessRunnableTypeName), this would "just work", no DynamicDependency needed.

Type.GetType with a constant string gets fully analyzed and produces no trimming warnings.

DynamicDependency is always a last resort thing because it means there is code that cannot be analyzed somewhere and if that code ever changes and people forget about the DynamicDependency, that's a bug...

@MichalStrehovsky
Copy link
Member

@eliphatfs
Copy link
Contributor Author

eliphatfs commented Mar 3, 2025

Hi Michal, thank you for the valuable insights and comments! Are there any way through these intrinsics we may also solve the missing metadata error from MakeGeneric BenchmarkAction<T> calls where T is the return type of the methods users attributed with [Benchmark]?

@MichalStrehovsky
Copy link
Member

There are certain patterns where MakeGeneric might be AOT safe - those are documented at https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/intrinsic-requiresdynamiccode-apis#methodinfomakegenericmethodtype-method-net-9 (note this is a different doc from above - one is for trimming, the other is for AOT).

If it involved valuetypes, this can only be safe if analysis can see all possible instantiations ahead of time.

So typeof(SomeType).GetMethod("GenericMethod").MakeGenericMethod(typeof(int)) is safe, or typeof(SomeType).GetMethod("GenericMethod").MakeGenericMethod(typeof(T)) is safe, but if the type is obtained from a place like "return type of a method annotated Benchmark", there's no way to express that.

You'd need a source generator that generates the MakeGeneric calls in a shape that can be analyzed (or better yet, just source-generate the C# to construct a delegate to the instantiated method if it's visible/accessible instead of reflecting at runtime).

@eliphatfs
Copy link
Contributor Author

But just personally, as someone who has been using C# for many years yet not super familiar with dotnet runtime, I think using DynamicDependency looks more explicit and less error-prone than relying on internal analysis details of the compiler. We can add more tests to ensure there are no bugs in the case the reflection code changes.

@eliphatfs
Copy link
Contributor Author

I also agree using a source generator is the ultimate solution, just as Tim has mentioned above.

@MichalStrehovsky
Copy link
Member

relying on internal analysis details of the compiler

These are documented behaviors (documented on learn.microsoft.com on the pages I linked to) and as such are contractual, same way as e.g. Console.WriteLine is (but you can always p/invoke into printf if you don't trust WriteLine, same way as the DynamicDependency, it's just more awkward to do so).

If benchmark.net means AOT support seriously, eventually it will enable the trimming/AOT analyzers and then the Assembly.GetType use would immediately get flagged as a warning. Rewriting it to use Type.GetType with a constant string would clear that warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InProcessNoEmitToolchain + NativeAOT Support?
3 participants