Add Span SequenceCompareTo extension method - #26232
Conversation
| Debug.Assert(secondLength >= 0); | ||
|
|
||
| if (Unsafe.AreSame(ref first, ref second)) | ||
| goto Equal; |
There was a problem hiding this comment.
return firstLength.CompareTo(secondLength); can be right here. I do not think you are getting anything by jumping to it.
There was a problem hiding this comment.
I would say this "AreSame" will be optimization only in very rare cases, but if you see it as useful, then goto solution will generate shorter native code, so I am for leaving it there.
| int result = Unsafe.AddByteOffset(ref first, i).CompareTo(Unsafe.AddByteOffset(ref second, i)); | ||
| if (result != 0) return result; | ||
| i += 1; | ||
| } |
There was a problem hiding this comment.
Direct return 0; would be better, I think.
There was a problem hiding this comment.
Where would I return 0? After the loop ends, we may still return other values.
There was a problem hiding this comment.
After the loop ends, you know that the two sequences are 100% equal. No?
There was a problem hiding this comment.
Sorry, I have misread this ... I though that the first statement is comparing the lengths.
There was a problem hiding this comment.
Your implementation was fine as it was. (Also ignore the other comment.)
There was a problem hiding this comment.
No, minLength is the length of the smaller sequence. The lengths could be different still.
| } | ||
|
|
||
| Equal: | ||
| return firstLength.CompareTo(secondLength); |
There was a problem hiding this comment.
I am for changing this to simple return firstLength-secondLength (because lengths could be only positive there cannot happen any bad overflow).
There was a problem hiding this comment.
I wanted the results here to be consistent with the existing int.CompareTo. Subtracting could result in values other than {-1, 1, 0}. I am not sure if the performance win is worth the inconsistency. Thoughts?
There was a problem hiding this comment.
The contract for Compare methods is to return positive, 0, negative. It is not necessary to normalize to -1,1,0 - other paths in your implementation do not normalize either.
There was a problem hiding this comment.
The contract for
Comparemethods is to return positive, 0, negative. It is not necessary to normalize to -1,1,0
Fair point
other paths in your implementation do not normalize either.
Where? Every comparison uses the CompareTo method (at least for T = byte), as far as I can tell.
There was a problem hiding this comment.
Byte.CompareTo method also does not return just -1, 1, 0 (it is also simple subtraction). Only int one have to due to possibility of overflow for negative inputs. And because of mispredicted comparisons in int.CompareTo overhead could be nontrivial, but it is harder to measure...
|
Is this PR good to go? |
* Add SequenceCompareTo initial * Update tests and remove use of Dangerous API * Add SequenceCompareTo performance test and loop unroll * Revert loop unrolling SequenceCompareTo * Vectorize SequenceCompareTo<byte> * Fix performance test class name. * Address PR feedback Commit migrated from dotnet/corefx@7931769
4 participants
Fixes https://github.com/dotnet/corefx/issues/16878
SequenceCompareTo<byte>implementationSequenceCompareTo<T>similar toSequenceEqual<T>since I observed no performance improvement (in fact there was some regression for T = int)cc @KrzysztofCwalina, @jkotas, @atsushikan, @AronParker, @Bobris