Skip to content

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Aug 31, 2025

  • Move throw helpers to ThrowHelpers
  • Reduce code size by boxing early

* Move throw helpers to `ThrowHelpers`
* Reduce code size by boxing values in cold path
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 31, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 31, 2025
@jkotas
Copy link
Member

jkotas commented Aug 31, 2025

I do not see any end-to-end improvements here, only regressions.

@MihaZupan
Copy link
Member

What's the intention behind optimizing the cold throw helpers?
The hot paths that call into them don't care about how much logic there is in them as they should never be inlined regardless.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Sep 1, 2025

What's the intention behind optimizing the cold throw helpers?

Purely to reduce code size, since the generic methods end up being instantiated with many different types.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Sep 1, 2025

they should never be inlined regardless.

I wonder why they are inlined for Vector<float>: https://csharp.godbolt.org/z/rqxa1q7G6

@tannergooding
Copy link
Member

Vector<T> is a special type that gives an inlining profitability boost when used/encountered. It's effectively special and this is probably an area that should be adjusted to say "don't do that if the path is known to be cold"

@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

@jkotas jkotas removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 29, 2025
Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

@xtqqczze thanks for attempts on improving this. Looking at MichalStrehovsky/rt-sz#188 this looks like it's regressing more than it optimizes. Please let us know if you'll be trying to improve this or if we should close this PR.

@jkotas jkotas marked this pull request as draft December 11, 2025 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Runtime community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants