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

Temporarily disable arm64 intrinsics in UTF-16 validation code paths #42052

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

GrabYourPitchforks
Copy link
Member

Fixes #41699.

Disables the ARM64-intrinsicified UTF-16 validation code paths since they ended up having poorer performance characteristics for non-ASCII data compared to their 3.x counterparts. With this PR, we skip the intrinsics entirely but stay within vectorized code as much as possible. The codegen and perf should be on-par with what shipped in 3.x.

Once this change is in, we can re-introduce tweaked versions of the arm64 code path, validate the perf numbers, and cherry-pick the "real" fix into the 5.x servicing branches.

/cc @jeffhandley

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @GrabYourPitchforks !

I've re-run the failed CI legs, as soon as they go green I squash the PR

@danmoseley danmoseley merged commit 0923e83 into dotnet:master Sep 10, 2020
@danmoseley
Copy link
Member

/backport to release/5.0-rc2

@github-actions
Copy link
Contributor

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/247917869

@danmoseley
Copy link
Member

@adamsitnik should someone verify the perf effect of this before back port is merged?

@adamsitnik
Copy link
Member

should someone verify the perf effect of this before back port is merged?

yes. I would love to do it, but I can't build the runtime repo on Windows ARM64 #42008 ;)

@danmoseley
Copy link
Member

Is it possible to cross compile corelib dll and copy it over? I know it's a hassle until we fix the issue you mention.

@danmoseley
Copy link
Member

All done thanks @carlossanlop

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

Successfully merging this pull request may close these issues.

[ARM64] Performance regression: Utf8Encoding
4 participants