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

Revert "? true : false" removal in ArraySortHelper.cs #70631

Merged
merged 1 commit into from
Jun 13, 2022

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jun 12, 2022

Reverts a change from #63095

Fixes #70373 perf regression
image

Unfortunately, the proper fix in the JIT is quite difficult at this point

Verified locally, cc @stephentoub

@ghost
Copy link

ghost commented Jun 12, 2022

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

Issue Details

Reverts a change from #63095

Fixes #70373 perf regression
image

Unfortunately, the proper fix in the JIT is quite difficult at this point

Verified locally.

Author: EgorBo
Assignees: -
Labels:

area-System.Collections

Milestone: -

@danmoseley
Copy link
Member

Is there an issue tracking this JIT limitation?

@am11
Copy link
Member

am11 commented Jun 12, 2022

Is there an issue tracking this JIT limitation?

It was #4207, we should keep it open. It has good discussion / context.
forwardsub still does not handle these cases.

@EgorBo
Copy link
Member Author

EgorBo commented Jun 12, 2022

Essentialy, the issue is the same as in here #65327 - JIT doesn't do a good job "unraveling" branches.

@EgorBo EgorBo merged commit a103efd into dotnet:main Jun 13, 2022
@stephentoub
Copy link
Member

Why are we reverting just this one instance of the change? We have verification all the others didn't regress (beyond lack of perf test regressions)?

@EgorBo
Copy link
Member Author

EgorBo commented Jun 13, 2022

Why are we reverting just this one instance of the change? We have verification all the others didn't regress (beyond lack of perf test regressions)?

For instance, these ArraySortHelper helpers were fully covered with ? true : false:
image

but as it turned out for primitives they're not needed any more due to Andy's PR, only CompareTo() is tricky.
There were only a few minor diffs in that PR so it sounded like a good tread-off to drop these hacks.

@EgorBo EgorBo deleted the revert-arraysort branch June 13, 2022 11:39
@stephentoub
Copy link
Member

only CompareTo() is tricky

Ok, so we reverted all CompareTo ones and believe that there were no other diffs in the original change.

@EgorBo
Copy link
Member Author

EgorBo commented Jun 13, 2022

only CompareTo() is tricky

Ok, so we reverted all CompareTo ones and believe that there were no other diffs in the original change.

There are more 😞 I am investigating root causes of them just to have better understanding what exactly we're missing in jit - might do a full revert

@ghost ghost locked as resolved and limited conversation to collaborators Jul 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Perf regressions in sorting
4 participants