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

Fix bad inlining in DateTime #56466

Merged
merged 1 commit into from
Jul 30, 2021
Merged

Fix bad inlining in DateTime #56466

merged 1 commit into from
Jul 30, 2021

Conversation

pentp
Copy link
Contributor

@pentp pentp commented Jul 28, 2021

Recent JIT inlining changes meant that some fallback methods got inlined now.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 28, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@EgorBo
Copy link
Member

EgorBo commented Jul 28, 2021

IsValidTimeWithLeapSeconds is inlined because callsites provide a constant argument for kind so it finds two foldable branches + additional multiplier for methods returning structs by value.

With dynamic PGO all these fallbacks will never be inlined because of low real weight, but it's not the case for the built-in profile or no-profile modes.
Still, lgtm, thanks for noticing that!

@pentp
Copy link
Contributor Author

pentp commented Jul 30, 2021

Can this be merged (I don't have write access)? The test failures are really unlikely to be caused by adding NoInlining...

@stephentoub stephentoub merged commit 3d61a00 into dotnet:main Jul 30, 2021
@pentp pentp deleted the dt-inlining branch July 30, 2021 16:49
@ghost ghost locked as resolved and limited conversation to collaborators Aug 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

None yet

3 participants