Fix perf regression in IntPtr operators on 32-bit platforms #41198
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
I don't think I can review this immediately -- my brain is tired. I'm impressed that this affected perf but not correctness -- how did it regress perf? |
|
The code before this change is equivalent to:
|
|
Where was IntPtr involved in these simple collection lookups? |
|
Ah I see the other PR |
Switching to C# built-in uint/nuint types caused these operators to use long/ulong IntPtr/UIntPtr constructors instead of int/uint IntPtr constructors that it were used before. The fix is to avoid going through the IntPtr/UIntPtr constructors and just depend on the built-in uint/nuint implicit conversions. Fixes dotnet#41167
There was a problem hiding this comment.
I've built @jkotas branch and verified that the fix is solving the regression:
| Method | Size | 3.1 | 5.0 | #41198 |
|---|---|---|---|---|
| Array | 512 | 36.196 us | 115.129 us | 35.911 us |
| Span | 512 | 33.661 us | 124.819 us | 32.897 us |
| List | 512 | 35.549 us | 113.872 us | 35.540 us |
| ICollection | 512 | 34.857 us | 116.611 us | 34.810 us |
| LinkedList | 512 | 212.843 us | 213.416 us | 212.328 us |
| HashSet | 512 | 8.303 us | 4.670 us | 4.750 us |
| Queue | 512 | 36.406 us | 115.413 us | 35.455 us |
| Stack | 512 | 31.660 us | 27.987 us | 28.409 us |
| SortedSet | 512 | 31.978 us | 29.323 us | 30.133 us |
| ImmutableArray | 512 | 37.382 us | 116.364 us | 37.401 us |
| ImmutableHashSet | 512 | 28.201 us | 27.993 us | 27.877 us |
| ImmutableList | 512 | 3,457.214 us | 693.476 us | 707.291 us |
| ImmutableSortedSet | 512 | 34.652 us | 37.129 us | 35.202 us |
It's amazing that such a small change could have caused such regression. @jkotas big thanks for a very quick fix!
|
/backport to release/5.0 |
|
Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/221717538 |
| @@ -148,15 +148,15 @@ public unsafe int ToInt32() | |||
|
|
|||
| [NonVersionable] | |||
| public static unsafe IntPtr operator +(IntPtr pointer, int offset) => | |||
| new IntPtr((nint)pointer._value + offset); | |||
There was a problem hiding this comment.
Isn't this still implicitly invoking the IntPtr ctor since it returns one? Your change somehow changes it to use the int one on x86?
There was a problem hiding this comment.
The C# compiler has implicit nint -> IntPtr conversion. This conversion is no-op. It does not generate any code.
There was a problem hiding this comment.
Ah, that was what I was missing. OK thanks LGTM
ghost
locked as resolved and limited conversation to collaborators
Switching to C# built-in uint/nuint types caused these operators to use long/ulong IntPtr/UIntPtr constructors instead of int/uint IntPtr constructors that it were used before.
The fix is to avoid going through the IntPtr/UIntPtr constructors and just depend on the built-in uint/nuint implicit conversions.
Fixes #41167