Optimize BitArray APIs using span and bitwise operations #33367
Conversation
src/System.Text.Json/src/System/Text/Json/CustomUncheckedBitArray.cs
Outdated
Show resolved
Hide resolved
ahsonkhan
changed the title
| public bool Get(int index) | ||
| { | ||
| if (index < 0 || index >= Length) | ||
| if ((uint)index >= (uint)Length) |
There was a problem hiding this comment.
Why can’t the JIT optimise cases like this? I honestly have seen so many code changes that do this that engineering effort should go to ensuring wins across the board with these sorts of checks!
There was a problem hiding this comment.
This particular one is tricky to handle in the JIT. If index < 0 is true then index >= Length is no longer evaluated. If this is null (the runtime does not guarantee that it is not) then a NullReferenceException is avoided. This means that combining the 2 comparisons will change the meaning of the code when this is null. Kind of sucks but it is what it is.
Perhaps we should assume that this cannot be null. C# tries to prevent that by using callvirt to invoke methods. Other languages.... dunno.
The JIT could handle cases like c >= '0' && c <= '9', I've actually done that in the past. But there weren't many of them in the framework so I didn't bother to finish it.
There was a problem hiding this comment.
It could be argued that Roslyn should be doing such optimizations (and it actually does for switch cases, e.g., when you have case '0' to case '9' with fallthroughs, it generates the subtraction and unsigned compare).
But then again in a general comparison like here it could end up hurting performance if for example it's common for index to be negative...
No. I recently migrated all possible performance projects to netstandard to make them runnable on both netcoreapp and netfx. What do you mean by "test results are running on netstandard"? You can't run on netstandard. |
OK. Good to know.
Yep, you are correct. I made a mistake and hence my comment can be ignored. I thought I was running on netfx by default (which uses portable span). Thanks for clarifying. Edited the original post. |
|
Unrelated test failure: https://github.com/dotnet/corefx/issues/24355 Ubuntu.1604.Arm64.Open-arm64-Release |
| ((bytes[j + 2] & 0xff) << 16) | | ||
| ((bytes[j + 3] & 0xff) << 24); | ||
| j += 4; | ||
| m_array[i] = MemoryMarshal.Read<int>(byteSpan); |
There was a problem hiding this comment.
This will break Mono. It does not work on big-endian platforms. Use BinaryPrimitives.ReadInt32LittleEndian instead.
.
There was a problem hiding this comment.
Why did it work before when we were directly or'ing elements of the byte array?
Regardless of endianness, we had the following before, no?
byte array: [a, b, c, d]
resulting int: [a | b << 8 | c << 16 | d << 24] => [d c b a]
Why isn't the resulting int incorrect for big-endian? What am I missing here?
Don't we need to swap the ordering for big-endian?
resulting int: [d | c << 8 | b << 16 | a << 24] => [a b c d]
Edit: Never mind. The bytes in the byte array themselves are stored in reverse ordering when we are on big-endian platforms. That's why we didn't need to reverse the or'ing order ourselves.
There was a problem hiding this comment.
Never mind. The bytes in the byte array themselves are stored in reverse ordering when we are on a big-endian platforms. That's why we didn't need to reverse the or'ing order ourselves.
Right.
There was a problem hiding this comment.
Is there a way to add tests or CI legs that simulate big-endian platforms so that we can catch such mistakes systematically?
There was a problem hiding this comment.
@dotnet/dnceng, is it possible to add a big-endian CI leg, even if it is on demand only?
There was a problem hiding this comment.
@ahsonkhan This is not a problem for dnceng to solve. .NET Core or CoreFX does not run on any big-endian platform today directly. Bigendian systems are supported by Mono only. We would need to do work to build CoreFX for Mono first.
There was a problem hiding this comment.
This is not a problem for dnceng to solve.
Pardon my ignorance (and the ping). Thanks for clarifying, @jkotas. Good to know.
|
@dotnet-bot test OSX x64 Debug Build |
* Use a custom BitArray implementation instead of a stack to be more memory efficient * Fix BitArray perf test * Fix CopyTo tests - different exception thrown * Add tests for custom bit array and add details to debug assert * Revert JSON related changes to isolate BitArray changes * Revert leftover changes to Json * Address PR feedback * Fix bug in CopyTo * Avoid if check for lengths < 4. * Remove unnecessary bit operations in CopyTo * Revert back to using Array.Resize * Fix nits and address PR feedback. * Address PR feedback. * Remove use of Span.CopyTo and revert back to Array.Copy * Use BinaryPrimitives instead of MemoryMarshal to account for endianness.
…fx#33367) * Use a custom BitArray implementation instead of a stack to be more memory efficient * Fix BitArray perf test * Fix CopyTo tests - different exception thrown * Add tests for custom bit array and add details to debug assert * Revert JSON related changes to isolate BitArray changes * Revert leftover changes to Json * Address PR feedback * Fix bug in CopyTo * Avoid if check for lengths < 4. * Remove unnecessary bit operations in CopyTo * Revert back to using Array.Resize * Fix nits and address PR feedback. * Address PR feedback. * Remove use of Span.CopyTo and revert back to Array.Copy * Use BinaryPrimitives instead of MemoryMarshal to account for endianness. Commit migrated from dotnet/corefx@92a697c
13 participants
Note:
This performance test results are running on netstandard. I will retry on netcoreapp soon.I think we should change the perf test config to netcoreapp. Is that fine?Edit: Keeping perf tests as netstandard. See #33367 (comment)
cc @adamsitnik (we should probably port these to BDN).
cc @bartonjs, @stephentoub @safern