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

Avoid boxing ArgumentNullException.ThrowIfNull arguments in Tier-0 #104512

Closed
jkotas opened this issue Jul 6, 2024 · 7 comments · Fixed by #104815
Closed

Avoid boxing ArgumentNullException.ThrowIfNull arguments in Tier-0 #104512

jkotas opened this issue Jul 6, 2024 · 7 comments · Fixed by #104815
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@jkotas
Copy link
Member

jkotas commented Jul 6, 2024

ArgumentNullException.ThrowIfNull can incur boxing when applied to argument of generic type. Tier-1 JIT optimizations are able to optimize this boxing in steady state, but Tier-0 JIT optimization are not. It can result into excessive allocations during startup.

@dotnet/jit-contrib What would it take to augment Tier-0 to avoid boxing in this case?

From @AndyAyersMS: We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

More context and discussion of alternatives:

#104504
#82227
#100406

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 6, 2024
Copy link
Contributor

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

@julealgon
Copy link

We'd have to mark the method as intrinsic and add new logic to impBoxPatternMatch.

I assume these are not library-level changes, but JIT/compiler special casing of some sort?

Would it be possible to "fix" this problem purely via library modifications instead?

The reason I ask is that I'd like to be able to expand on existing validations with .NET9/C#13 extension types in the future and have them be as efficient as the native method if possible.

If the team decides for a special case for the method, then that makes it impossible for library authors and consumers to leverage the same benefits with their own extensions later (correct me if I'm wrong here).

@jkotas
Copy link
Member Author

jkotas commented Jul 8, 2024

Would it be possible to "fix" this problem purely via library modifications instead?

This was discussed in #104504 (comment) . The library-only fix would be to add generic overload. However, the startup runtime cost of such generic overload for ArgumentNullException.ThrowIfNull method would be more in a typical app than the cost of the occasional boxing. The cure would be worse than the disease.

@JulieLeeMSFT JulieLeeMSFT removed the untriaged New issue has not been triaged by the area owner label Jul 8, 2024
@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Jul 8, 2024
@julealgon
Copy link

Would it be possible to "fix" this problem purely via library modifications instead?

This was discussed in #104504 (comment) . The library-only fix would be to add generic overload. However, the startup runtime cost of such generic overload for ArgumentNullException.ThrowIfNull method would be more in a typical app than the cost of the occasional boxing. The cure would be worse than the disease.

@tarekgh , @jkotas mentions this on that message:

Is there a way to make this generic overload bind to generic types only? If we just add a generic overload, it would be preferred over the object overload, and we would end up with a ton of generic instantiations. The startup cost of these generic instantiations during startup of a typical app would be likely a lot more than the cost of the occasional boxing caused by this during startup of a typical app.

Isn't this concern now taken care of by the new attribute that is being introduced to control overload resolution order? Can't we now use that attribute (or extend its capabilities) to support this scenario of only picking the generic overloads when trying to match from a generic type?

@stephentoub
Copy link
Member

Isn't this concern now taken care of by the new attribute that is being introduced to control overload resolution order?

No:
#104504 (comment)

@julealgon
Copy link

Isn't this concern now taken care of by the new attribute that is being introduced to control overload resolution order?

No: #104504 (comment)

What about making the priority be conditional then @stephentoub ? Would that be feasible?

I understand the current implementation doesn't allow it, but what if it was expanded to? Surely the point where the decision is made has the required information, it would just be a matter of exposing that to the "priority setter" to allow for customization based on whether the type is generic or not? Or if the team doesn't want to open the API that much, just maybe have an extra boolean property in the attribute to control the applicability of the priority to generic types etc.

Wouldn't that be preferable to JIT-level optimizations in this case since that would provide the benefit to all library use cases?

@stephentoub
Copy link
Member

stephentoub commented Jul 8, 2024

What about making the priority be conditional then @stephentoub ? Would that be feasible?

You're talking about revising/augmenting a language feature (and one which is part of one of the most complicated parts of the language, that of overload resolution). That would need to be proposed and discussed over in dotnet/csharplang.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants