Skip to content

Vectorize IndexOfAnyExcept<T>(T value) - #73488

Merged
stephentoub merged 3 commits into
dotnet:mainfrom
stephentoub:vectorizeindexofany1
Aug 10, 2022
Merged

Vectorize IndexOfAnyExcept<T>(T value)#73488
stephentoub merged 3 commits into
dotnet:mainfrom
stephentoub:vectorizeindexofany1

Conversation

@stephentoub

Copy link
Copy Markdown
Member

Contributes to #67942

[Params(1,4,16,64,256,1024)]publicintLength{get;set;}privatebyte[]_zeros;[GlobalSetup]publicvoidSetup()=>_zeros=newbyte[Length];[Benchmark]publicboolAllZeroTrue()=>_zeros.AsSpan().IndexOfAnyExcept((byte)0)<0;
MethodToolchainLengthMeanRatioCode Size
AllZeroTrue\main\corerun.exe10.6482 ns1.0072 B
AllZeroTrue\pr\corerun.exe11.7013 ns2.62242 B
AllZeroTrue\main\corerun.exe42.6035 ns1.0072 B
AllZeroTrue\pr\corerun.exe43.5931 ns1.38242 B
AllZeroTrue\main\corerun.exe164.8521 ns1.0072 B
AllZeroTrue\pr\corerun.exe162.3925 ns0.49242 B
AllZeroTrue\main\corerun.exe6418.6816 ns1.0072 B
AllZeroTrue\pr\corerun.exe643.5615 ns0.19242 B
AllZeroTrue\main\corerun.exe25681.8503 ns1.0072 B
AllZeroTrue\pr\corerun.exe25610.3610 ns0.13242 B
AllZeroTrue\main\corerun.exe1024310.6106 ns1.0072 B
AllZeroTrue\pr\corerun.exe102437.1116 ns0.12242 B

The one thing that makes me a tad hesitant is the case of the except value being found at the very beginning, in which case this does incur a (very small) penalty even for really long inputs, e.g.

[Params(1024)]publicintLength{get;set;}privatebyte[]_allBitsSet;[GlobalSetup]publicvoidSetup()=>_allBitsSet=Enumerable.Repeat((byte)0xFF,Length).ToArray();[Benchmark]publicboolAllBitsSetLookingForNon0()=>_allBitsSet.AsSpan().IndexOfAnyExcept((byte)0)<0;
MethodToolchainLengthMeanRatioRatioSDCode Size
AllBitsSetLookingForNon0\main\corerun.exe10240.4979 ns1.000.0072 B
AllBitsSetLookingForNon0\pr\corerun.exe10241.7421 ns3.510.11242 B

I'm not sure what if anything to do about it.

@stephentoubstephentoub added this to the 7.0.0 milestone Aug 5, 2022
@ghostghost added the area-System.Memory label Aug 5, 2022
@ghostghost assigned stephentoubAug 5, 2022
@stephentoubstephentoub changed the title Vectorize IndexOfAnyException<T>(T value)Vectorize IndexOfAnyExcept<T>(T value)Aug 5, 2022
@ghost

ghost commented Aug 5, 2022

Copy link
Copy Markdown

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

Issue Details

Contributes to #67942

[Params(1,4,16,64,256,1024)]publicintLength{get;set;}privatebyte[]_zeros;[GlobalSetup]publicvoidSetup()=>_zeros=newbyte[Length];[Benchmark]publicboolAllZeroTrue()=>_zeros.AsSpan().IndexOfAnyExcept((byte)0)<0;
MethodToolchainLengthMeanRatioCode Size
AllZeroTrue\main\corerun.exe10.6482 ns1.0072 B
AllZeroTrue\pr\corerun.exe11.7013 ns2.62242 B
AllZeroTrue\main\corerun.exe42.6035 ns1.0072 B
AllZeroTrue\pr\corerun.exe43.5931 ns1.38242 B
AllZeroTrue\main\corerun.exe164.8521 ns1.0072 B
AllZeroTrue\pr\corerun.exe162.3925 ns0.49242 B
AllZeroTrue\main\corerun.exe6418.6816 ns1.0072 B
AllZeroTrue\pr\corerun.exe643.5615 ns0.19242 B
AllZeroTrue\main\corerun.exe25681.8503 ns1.0072 B
AllZeroTrue\pr\corerun.exe25610.3610 ns0.13242 B
AllZeroTrue\main\corerun.exe1024310.6106 ns1.0072 B
AllZeroTrue\pr\corerun.exe102437.1116 ns0.12242 B

The one thing that makes me a tad hesitant is the case of the except value being found at the very beginning, in which case this does incur a (very small) penalty even for really long inputs, e.g.

[Params(1024)]publicintLength{get;set;}privatebyte[]_allBitsSet;[GlobalSetup]publicvoidSetup()=>_allBitsSet=Enumerable.Repeat((byte)0xFF,Length).ToArray();[Benchmark]publicboolAllBitsSetLookingForNon0()=>_allBitsSet.AsSpan().IndexOfAnyExcept((byte)0)<0;
MethodToolchainLengthMeanRatioRatioSDCode Size
AllBitsSetLookingForNon0\main\corerun.exe10240.4979 ns1.000.0072 B
AllBitsSetLookingForNon0\pr\corerun.exe10241.7421 ns3.510.11242 B

I'm not sure what if anything to do about it.

Author:stephentoub
Assignees:-
Labels:

area-System.Memory

Milestone:7.0.0

@stephentoub

Copy link
Copy Markdown
MemberAuthor

Failure is #73247

@stephentoub

Copy link
Copy Markdown
MemberAuthor

@adamsitnik, can you please review this? Thanks.

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thank you @stephentoub ! I really like the namings you use + lack of gotos ;)

@stephentoub

Copy link
Copy Markdown
MemberAuthor

Thanks for reviewing, Adam.

@tannergooding

Copy link
Copy Markdown
Member

I'm not sure what if anything to do about it.

I think this is an acceptable tradeoff. The measured difference is just over 1ns which would be about 2-4 clock cycles on most modern computers, this is likely representative of the jump to IndexOfAnyExceptValueType and branch cost for checking the length.

In practice, the actual spilling of args, call, and general memory latency is more than this so most users likely won't see a difference. It's likewise a general avenue we might want to look at improved JIT support around, for being able to ensure that cost for such checks can be minimized.

One think that might help would be making it so that you have a pattern like the following and marking the containing method as AggressiveInlining

if(length<Vector128<T>.Count){ScalarImpl();}else{VectorImpl();}

This would theoretically allow the JIT to hoist, constant fold, or otherwise mitigate the branch for various input kinds.

{
for (int i = 0; i < length; i++)
{
if (!Unsafe.Add(ref searchSpace, i).Equals(value0))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just wondering, do you have the disassembly the JIT emits for this?

I'd expect it emits a cmp reg, [addr] (where reg contains value0), but I know there were some issues with Unsafe.Add being converted to an [addr] before.

@stephentoub
stephentoub merged commit 301d8e0 into dotnet:mainAug 10, 2022
@stephentoub
stephentoub deleted the vectorizeindexofany1 branch August 10, 2022 20:55
@stephentoub

Copy link
Copy Markdown
MemberAuthor

this is likely representative of the jump to IndexOfAnyExceptValueType and branch cost for checking the length.

To be clear, I wasn't referring to the case where there are fewer than Count elements, rather the case where there are more than Count elements and the very first element doesn't match the specified value. I expect that to be a fairly common occurrence with some uses of this method.

@ghostghost locked as resolved and limited conversation to collaborators Sep 10, 2022
@EgorBo

Copy link
Copy Markdown
Member

Hm.. looks like this regressed System.Collections.Concurrent.IsEmpty<String>.Dictionary(Size: 512) but none regressions were filed
image

https://pvscmdupload.blob.core.windows.net/reports/allTestHistory%2frefs%2fheads%2fmain_x64_ubuntu%2018.04%2fSystem.Collections.Concurrent.IsEmpty(String).Dictionary(Size%3a%20512).html

(I blamed this PR because ConcurrentDictionary uses IndexOfAnyExcept for AreAllBucketsEmpty)

@stephentoub

Copy link
Copy Markdown
MemberAuthor

looks like this regressed System.Collections.Concurrent.IsEmpty.Dictionary(Size: 512) but none regressions were filed

By a nanosecond or two, right? That's largely expected. I'd expect IsEmpty to get a tad slower when the dictionary isn't empty and faster when it is empty; basically an extra branch or two in the check.

@EgorBo

Copy link
Copy Markdown
Member

I totally agree and didn't file an issue for it, it's just needed for perf study report

Sign up for freeto 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.

5 participants

@stephentoub@tannergooding@EgorBo@adamsitnik@MichalPetryka