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

Remove strict typeof byte/char dependencies in MemoryExtensions #20855

Merged

Conversation

Projects
None yet
5 participants
@GrabYourPitchforks
Copy link
Member

commented Nov 7, 2018

This allows many of the vectorized code paths in MemoryExtensions (IndexOf, Contains, etc.) to be optimized for Ts other than byte and char. For instance, sbyte and ushort will now qualify for the performance optimizations.

The real reason for this is to prepare for the introduction of the Utf8Char primitive type, which we want to receive many of the same optimizations as byte receives in the vectorized code paths. With this change, there's now only one location where the Utf8Char check has to be added (all the way at the end of the file), rather than sprinkling additional typeof(T) == typeof(Utf8Char) checks all throughout this file.

Ideally we'd also be able to detect enums in here just for completeness.

WIP - unit tests have not been written for this.

@GrabYourPitchforks

This comment has been minimized.

Copy link
Member Author

commented Nov 7, 2018

FYI, with this PR some methods like SequenceCompareTo still use direct typeof(byte) / typeof(char) comparisons. The reason for this is listed as a comment at the bottom of the file: the type check methods only say that the Equals method has a behavior consistent with byte / char; it doesn't say anything about CompareTo having such consistency.

@AndyAyersMS

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

You should definitely look at diffs for this -- the early branch folding done by the jit is more effective when the type tests are at top level rather then in some inlinee. And it's possible we may not end up back in the same place if we have to inline and then optimize.

@GrabYourPitchforks

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

@jkotas @AndyAyersMS What if we do the same trick we do for RuntimeHelpers.IsOrContainsReferences<T>()? Would implementing the IsTypeEquatableAsByte<T>() method in the VM solve the issue you both described?

@jkotas

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

It would work, but it does not feel right to add this helper.

The question I ask for helper like these that support a regular framework code every library out there should be able to write: Is it something that we would expose as public API? I do not think that IsTypeEquatableAsByte helper would be ever exposed as a public API.

Note that these runtime-implemented helpers have to be implemented up to 5 times, once for each runtime that uses the shared code (CoreCLR, CoreRT, ProjectN, Mono, Unity), so they are not "cheap".

@GrabYourPitchforks

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

If we wanted to generalize it, we'd actually want something akin to C++'s std::is_trivial, but with the additional constraint that the type is memcmp-comparable. Keeping with the pattern that I'm terrible at choosing method names:

internal static bool IsMemcmpComparable<T>();

The behavior is that it'd return true only for primitives, enums, and structs of these that use the base implementation of operator ==. Then the caller could check if (sizeof(T) == 1 && IsMemcmpComparable<T>()) { /* treat as BYTE */ }. The issue with this is that I don't know how we'd plumb Char8 / Rune / IntPtr / UIntPtr / etc. through here without special-casing them. Those types all implement operator ==, but they use a trivial equality comparison.

@jkotas

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Yes, something like that would be better.

@GrabYourPitchforks

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

Is there a better method for determining whether a non-primitive type is "trivially comparable" other than special-casing it? I suppose I could introduce an [IsTriviallyComparable] internal attribute.

@jkotas

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

I think we would just hardcode the logic in the runtime.

Allow fast-tracking more types through MemoryExtensions's methods
- Now supports sbyte, Char8, and others in addition to byte / char

@GrabYourPitchforks GrabYourPitchforks force-pushed the GrabYourPitchforks:memoryextensions_typeof branch from 1449013 to 46e5d48 Mar 16, 2019

@GrabYourPitchforks

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

@jkotas @AndyAyersMS I hard-coded the list of types and moved the logic into the VM. Also ran some quick sample apps with MemoryExtensions.Contains<ushort> and MemoryExtensions.SequenceEqual<Char8> to make sure they were going down their respective fast-path implementations. Is this along the lines of what you think a proper implementation would look like?

(I don't know if the particular changes I made to MemoryExtensions will put additional pressure on the JIT. I don't know how to measure that.)

Show resolved Hide resolved src/vm/jitinterface.cpp Outdated
Show resolved Hide resolved src/vm/jitinterface.cpp Outdated
@jkotas

jkotas approved these changes Mar 16, 2019

Copy link
Member

left a comment

LGTM otherwise

@AndyAyersMS

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

I don't see anything that would cause issues for the jit.

@jkotas

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

cc @marek-safar New runtime dependency from shared partition

@GrabYourPitchforks GrabYourPitchforks changed the title [WIP] Remove strict typeof byte/char dependencies in MemoryExtensions Remove strict typeof byte/char dependencies in MemoryExtensions Mar 16, 2019

@GrabYourPitchforks

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

@jkotas @marek-safar Is there anything I can do here that would avoid the break on the Mono side of the house when this goes over? Perhaps move this RuntimeHelpers method into the shared partition and give it a default implementation?

@marek-safar

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

@GrabYourPitchforks it's ok to have it as it's, although better name than quite confusing IsTriviallyComparable would be nice.

@jkotas

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

@marek-safar Do you have suggestion for better name?

Some ideas: IsTriviallyEquatable, EqualityComparesBytes

@marek-safar

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

The method does not compare anything so it should be something about the type, perhaps HasBlittableEquality or something like that

@GrabYourPitchforks

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2019

Thanks for the suggestions! I'm going to go with IsBitwiseEquatable for now since the IsXyz<T>() pattern matches other methods on RuntimeHelpers, and IMO "blittable" seems a bit too marshal-related.

GrabYourPitchforks added some commits Mar 17, 2019

@GrabYourPitchforks GrabYourPitchforks merged commit b88f2f6 into dotnet:master Mar 18, 2019

69 of 70 checks passed

CentOS7.1 x64 Checked Innerloop Build and Test Build finished.
Details
CentOS7.1 x64 Debug Innerloop Build Build finished.
Details
OSX10.12 x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked crossgen_comparison Build and Test Build finished.
Details
Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Release crossgen_comparison Build and Test Build finished.
Details
Ubuntu x64 Checked CoreFX Tests Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
WIP Ready for review
Details
Windows_NT x64 Checked CoreFX Tests Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release CoreFX Tests Build finished.
Details
Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x86 Release Innerloop Build and Test Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
coreclr-ci Build #20190317.76 succeeded
Details
coreclr-ci (Build Linux arm checked) Build Linux arm checked succeeded
Details
coreclr-ci (Build Linux arm debug) Build Linux arm debug succeeded
Details
coreclr-ci (Build Linux arm release) Build Linux arm release succeeded
Details
coreclr-ci (Build Linux arm64 checked) Build Linux arm64 checked succeeded
Details
coreclr-ci (Build Linux arm64 debug) Build Linux arm64 debug succeeded
Details
coreclr-ci (Build Linux arm64 release) Build Linux arm64 release succeeded
Details
coreclr-ci (Build Linux_musl arm64 checked) Build Linux_musl arm64 checked succeeded
Details
coreclr-ci (Build Linux_musl arm64 debug) Build Linux_musl arm64 debug succeeded
Details
coreclr-ci (Build Linux_musl arm64 release) Build Linux_musl arm64 release succeeded
Details
coreclr-ci (Build Linux_musl x64 checked) Build Linux_musl x64 checked succeeded
Details
coreclr-ci (Build Linux_musl x64 debug) Build Linux_musl x64 debug succeeded
Details
coreclr-ci (Build Linux_musl x64 release) Build Linux_musl x64 release succeeded
Details
coreclr-ci (Build Linux_rhel6 x64 checked) Build Linux_rhel6 x64 checked succeeded
Details
coreclr-ci (Build Linux_rhel6 x64 debug) Build Linux_rhel6 x64 debug succeeded
Details
coreclr-ci (Build Linux_rhel6 x64 release) Build Linux_rhel6 x64 release succeeded
Details
coreclr-ci (Build Windows_NT arm Checked) Build Windows_NT arm Checked succeeded
Details
coreclr-ci (Build Windows_NT arm Debug) Build Windows_NT arm Debug succeeded
Details
coreclr-ci (Build Windows_NT arm64 Checked) Build Windows_NT arm64 Checked succeeded
Details
coreclr-ci (Build Windows_NT arm64 Debug) Build Windows_NT arm64 Debug succeeded
Details
coreclr-ci (Build Windows_NT x64 Checked) Build Windows_NT x64 Checked succeeded
Details
coreclr-ci (Build Windows_NT x64 Debug) Build Windows_NT x64 Debug succeeded
Details
coreclr-ci (Build Windows_NT x86 Checked) Build Windows_NT x86 Checked succeeded
Details
coreclr-ci (Build Windows_NT x86 Debug) Build Windows_NT x86 Debug succeeded
Details
coreclr-ci (Test Pri0 Linux x64 checked) Test Pri0 Linux x64 checked succeeded
Details
coreclr-ci (Test Pri0 Linux_musl arm64 checked) Test Pri0 Linux_musl arm64 checked succeeded
Details
coreclr-ci (Test Pri0 OSX x64 checked) Test Pri0 OSX x64 checked succeeded
Details
coreclr-ci (Test Pri0 Windows_NT arm checked) Test Pri0 Windows_NT arm checked succeeded
Details
coreclr-ci (Test Pri0 Windows_NT arm64 checked) Test Pri0 Windows_NT arm64 checked succeeded
Details
coreclr-ci (Test Pri0 Windows_NT x64 checked) Test Pri0 Windows_NT x64 checked succeeded
Details
coreclr-ci (Test Pri0 Windows_NT x86 checked) Test Pri0 Windows_NT x86 checked succeeded
Details
coreclr-ci (Test pri0 Linux arm checked) Test pri0 Linux arm checked succeeded
Details
coreclr-ci (Test pri0 Linux arm64 checked) Test pri0 Linux arm64 checked succeeded
Details
coreclr-ci (Test pri0 Linux_musl x64 checked) Test pri0 Linux_musl x64 checked succeeded
Details
coreclr-ci (Test pri0 Linux_rhel6 x64 checked) Test pri0 Linux_rhel6 x64 checked succeeded
Details
coreclr-ci (build Linux x64 Checked) build Linux x64 Checked succeeded
Details
coreclr-ci (build Linux x64 Debug) build Linux x64 Debug succeeded
Details
coreclr-ci (build Linux x64 Release) build Linux x64 Release succeeded
Details
coreclr-ci (build OSX x64 Checked) build OSX x64 Checked succeeded
Details
coreclr-ci (build OSX x64 Debug) build OSX x64 Debug succeeded
Details
coreclr-ci (build OSX x64 Release) build OSX x64 Release succeeded
Details
coreclr-ci (build Windows_NT arm Release) build Windows_NT arm Release succeeded
Details
coreclr-ci (build Windows_NT arm64 Release) build Windows_NT arm64 Release succeeded
Details
coreclr-ci (build Windows_NT x64 Release) build Windows_NT x64 Release succeeded
Details
coreclr-ci (build Windows_NT x86 Release) build Windows_NT x86 Release succeeded
Details
license/cla All CLA requirements met.
Details

@GrabYourPitchforks GrabYourPitchforks deleted the GrabYourPitchforks:memoryextensions_typeof branch Mar 18, 2019

dschinde added a commit to dschinde/coreclr that referenced this pull request Apr 28, 2019

Improve pref of `Array.IndexOf()` for certain `T`.
Applies changes to `Array.IndexOf()` and `Array.LastIndexOf()` similar
to the changes made in dotnet#20855, so that types other than `byte` and
`char` can use use the fast vectorized path.

Also allows 32-bit and 64-bit types for which
`RuntimeHelpers.IsBitwiseEquatable<T>()` returns `true` to use the
faster implementation of `IndexOf` and `LastIndexOf` from
`MemoryExtensions`.

jkotas added a commit that referenced this pull request Apr 30, 2019

Improve pref of `Array.IndexOf()` for certain `T`. (#24293)
Applies changes to `Array.IndexOf()` and `Array.LastIndexOf()` similar
to the changes made in #20855, so that types other than `byte` and
`char` can use use the fast vectorized path.

Also allows 32-bit and 64-bit types for which
`RuntimeHelpers.IsBitwiseEquatable<T>()` returns `true` to use the
faster implementation of `IndexOf` and `LastIndexOf` from
`MemoryExtensions`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.