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

Mark FirstCharInUInt32IsAscii with aggressiveinlining #84306

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Apr 4, 2023

This small method pops up in CPU traces for MinimalApi benchmark.
With this change I see up ~5% improvement for a simple Utf8.FromUtf16 benchmark for small inputs (<32 bytes).

There are two problems here:

  1. ILLink doesn't get rid of BitConverter.IsLittleEndian, see ILLink substitute BitConverter.IsLittleEndian as true constant #83169
  2. JIT's inliner assigns big weights for ldsfld (it's afraid of potential helper calls)

I was playing with a fix for inliner but it needs a new JIT-EE API + diffs were huge and needs careful analysis. So for now let's slap an AggressiveInlining on it.

@ghost
Copy link

ghost commented Apr 4, 2023

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

Issue Details

This small method pops in CPU traces for MinimalApi benchmark.
With this change I see up ~5% improvement for a simple Utf8.FromUtf16 benchmark for small inputs (<32 bytes).

There are two problems here:

  1. ILLink doesn't get rid of BitConverter.IsLittleEndian, see ILLink substitute BitConverter.IsLittleEndian as true constant #83169
  2. JIT's inliner assigns big weights for ldsfld (it's afraid of potential helper calls)

I was playing with a fix for inliner but it needs a new JIT-EE API + diffs were huge and needs careful analysis. So for now let's slap an AggressiveInlining on it.

Author: EgorBo
Assignees: EgorBo
Labels:

area-System.Text.Encoding

Milestone: -

@@ -69,6 +69,7 @@ private static int GetIndexOfFirstNonAsciiByteInLane_AdvSimd(Vector128<byte> val
/// </summary>
/// <param name="value"></param>
/// <returns></returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
Copy link
Member

@stephentoub stephentoub Apr 4, 2023

Choose a reason for hiding this comment

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

Does the JIT do any better by default if it's instead implemented as:

return BitConverter.IsLittleEndian ?
    (value & 0xFF80u) == 0 :
    (value & 0xFF800000u) == 0;

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly, no:

image

Copy link
Member

Choose a reason for hiding this comment

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

Elsewhere in corelib we'll use:

#if BIGENDIAN

Should we do the same thing here instead and not rely on the linker at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably? I think we'll fix that in JIT's inliner eventually so up to you whatever looks better in code

@EgorBo EgorBo merged commit f2d5967 into dotnet:main Apr 4, 2023
@EgorBo EgorBo deleted the aggressive-inlining-narrow branch April 4, 2023 23:04
@ghost ghost locked as resolved and limited conversation to collaborators May 5, 2023
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.

3 participants