Skip to content

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 ghost assigned EgorBo Apr 4, 2023
@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: -

/// </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