Use string.IsNullOrEmpty to eliminate bounds check to first char#17512
Conversation
|
cc: @AndyAyersMS |
|
Sheesh, a never ending stream of hacks because the JIT can't figure things out. I need to figure out a way to clone myself and fix it... |
I suggest you post code that actually uses |
Yeah that's worse than either example |
|
Try |
|
Could the same kind of precondition check also help with |
That one contains a trivial for loop where the JIT should have no problem eliminating the range check. Besides, it does not inline so whatever happens inside it won't help the JIT improve the caller method in any way. |
Sounds like you are describing most of my PR aspnet/KestrelHttpServer#2347 😉 or as @halter73 refers to them "more esoteric parts [...] added to avoid extra bounds checks by the JIT." A little bit here and there that the Jit misses does add up. The changes in the PR are a gain of 100K rps or a gain of the performance of an entire IIS server of the same spec O_o |
Yeah. Now I haven't starred at everything to tell if it's feasible to handle everything in the JIT. But there definitely are things that the/a JIT could do if some of it's inner workings weren't so sloppy. And that's just disappointing. |
|
asm changes |
|
Looking at G_M59730_IG02:
test rdi, rdi
je SHORT G_M59730_IG03
cmp dword ptr [rdi+8], 0
- sete al
- movzx rax, al
- test eax, eax
- je SHORT G_M59730_IG08
+ ja SHORT G_M59730_IG08
G_M59730_IG03:
test rsi, rsi
je SHORT G_M59730_IG04
cmp dword ptr [rsi+8], 0
- sete al
- movzx rax, al
- test eax, eax
- je SHORT G_M59730_IG06
+ ja SHORT G_M59730_IG06
G_M59730_IG04:
mov rax, qword ptr [(reloc)]
mov rax, gword ptr [rax]
-; Total bytes of code 168, prolog size 8 for method String:Concat(ref,ref):ref
+; Total bytes of code 144, prolog size 8 for method String:Concat(ref,ref):ref |
So basically the improvement is due to the |
@benaadams, I see changes in that PR besides ones to work around things the JIT could have done. How much of that 100K rps is due to which changes? |
About 50% from skipping struct copies and some better implementations; and 50% from bounds checks (per host header char) |
|
Single threaded; focused microbenchmark, dev machine Inital Skip struct copies (improve implementations) plus bounds check eliminations Actual server on network Inital After PR |
Thanks. So if I'm reading the numbers correctly, if the JIT had been able to do a better job on bounds check removal, that would have improved throughput by ~2.5%, correct? |
Yes
True 😄 But if you next action was to then check the first char of the non-empty string, it now eliminates that bounds check 😉 |
What its doing per loop is pretty light (char validation) so the range check is a significant portion of it Though you'd get a similar effect if you searched for a char in a non-unsafe way in a string; not starting a zero offset and not ending at length public int IndexOf(char c, String s, int offset, int count)
{
var endPos = offset + count;
var i = offset;
for (; i < endPos; i++)
{
if (s[i] == c) break;
}
return i == endPos ? -1 : i;
}; loop start
L005a: cmp eax, [r8+0x8] ; range check
L005e: jae L008b ; out of bounds
L0060: movsxd r9, eax ; s[i]
L0063: movzx r9d, word [r8+r9*2+0xc] ; s[i]
L0069: movzx r10d, dx ; s[i]
L006d: cmp r9d, r10d ; check char
L0070: jz L0078 ; found, exit
L0072: inc eax ; i++
L0074: cmp eax, ecx ; i < endPos
L0076: jl L005a ; loop repeatWhere a lot more (half?) could be hoisted out of the loop |
|
Thanks for the example. It is important for us to see cases where even the small details matter. |
|
Realized I should validate the range so it could eliminate the bounds check public int IndexOf(char c, String s, int offset, int count)
{
var endPos = offset + count;
if ((uint)offset >= (uint)s.Length || (uint)count >= (uint)(s.Length - offset))
throw new ArgumentOutOfRangeException();
var i = offset;
for (; i < endPos; i++)
{
if (s[i] == c) break;
}
return i == endPos ? -1 : i;
}Same result ; loop start
L0072: cmp eax, r10d ; range check (validated by arg check)
L0075: jae L00c7 ; out of bounds (validated by arg check)
L0077: movsxd r9, eax ; s[i] Needs 3 movs?
L007a: movzx r9d, word [r8+r9*2+0xc] ; s[i] some could
L0080: movzx r11d, dx ; s[i] be hoisted?
L0084: cmp r9d, r11d ; check char
L0087: jz L008f ; found, exit
L0089: inc eax ; i++
L008b: cmp eax, ecx ; i < endPos
L008d: jl L0072 ; loop repeatTop half of the loop body could be changed to a single mov? |
| // value.Length == 0 as it will elide the bounds check to | ||
| // the first char: value[0] if that is performed following the test | ||
| // for the same test cost. | ||
| return (value == null || 0u >= (uint)value.Length) ? true : false; |
There was a problem hiding this comment.
The comment explains the use of 0u, but why use the (redundant) ternary operator instead of:
return (value == null || 0u >= (uint)value.Length);There was a problem hiding this comment.
That does this change
cmp dword ptr [rsi+8], 0
- sete al
- movzx rax, al
- test eax, eaxThere was a problem hiding this comment.
That does this change
Thanks! I think we should highlight that in the comments too (to avoid accidentally reverting it in the future).
There was a problem hiding this comment.
| /// It can be used for pinning and is required to support the use of span within a fixed statement. | ||
| /// </summary> | ||
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| public unsafe ref T GetPinnableReference() => ref (_length != 0) ? ref _pointer.Value : ref Unsafe.AsRef<T>(null); |
There was a problem hiding this comment.
Had rebased to do asm diffs against master; forgot so it made the sync bad
benaadams
force-pushed
the
IsNullOrEmpty-
branch
from
b37f912 to
84848f6
Compare
|
@dotnet-bot test Windows_NT x64 Checked corefx_baseline |
|
That overtriggered CI O_o @jkotas is this change ok for 2.1? |
CarolEidt
left a comment
CarolEidt
left a comment
There was a problem hiding this comment.
This change seems quite safe, but I'm assuming that we will defer to 2.2.
@benaadams @stephentoub @mikedn @AndyAyersMS please speak up if you think it should be in 2.1.
|
I'd love it to be in 2.1 as Kestrel has the exact pattern this would effect: if (string.IsNullOrEmpty(hostText))
{
return true;
}
if (hostText[0] == '[')
{
return IsValidIPv6Host(hostText);
}/cc @halter73 @davidfowl |
OK, I'll let @ahsonkhan or @stephentoub make that call. |
|
I really don't like this kind of code contortions but the problem avoided by |
Changing |
jkotas
left a comment
jkotas
left a comment
There was a problem hiding this comment.
I do not like hacks like these either. This one is simple enough and local enough that it is ok to take for 2.1.
9 participants
From aspnet/KestrelHttpServer#2347 (comment)
vs