Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Improve performance of Convert.ToInt32(bool) and related functions #16138

Closed
wants to merge 2 commits into from
Closed

Improve performance of Convert.ToInt32(bool) and related functions #16138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0679a70
Add bool-to-byte JIT helper
GrabYourPitchforks Jan 31, 2018
b14e82f
Faster Convert.ToInt32 implementation
GrabYourPitchforks Jan 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions src/mscorlib/shared/System/Convert.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,9 @@ public static sbyte ToSByte(object value, IFormatProvider provider)
[CLSCompliant(false)]
public static sbyte ToSByte(bool value)
{
return value ? (sbyte)Boolean.True : (sbyte)Boolean.False;
// Ideally we'd call BoolToByte(!!value) to normalize any true value to 1,
// but JIT optimizes !! away. The pattern below defeats this optimization.
return (sbyte)JitHelpers.BoolToByteNonNormalized(JitHelpers.BoolToByteNonNormalized(value) != 0);
}

[CLSCompliant(false)]
Expand Down Expand Up @@ -769,7 +771,9 @@ public static byte ToByte(object value, IFormatProvider provider)

public static byte ToByte(bool value)
{
return value ? (byte)Boolean.True : (byte)Boolean.False;
// Ideally we'd call BoolToByte(!!value) to normalize any true value to 1,
// but JIT optimizes !! away. The pattern below defeats this optimization.
Copy link
Member

Choose a reason for hiding this comment

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

How can JIT optimize !! away? Is not it a bug?

Copy link

Choose a reason for hiding this comment

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

Hmm, haven't checked but both the JIT and the C# compiler tend to assume that bool is either 0 or 1. It's a bit of the bug since the IL spec doesn't actually guarantee that but then this behavior is old enough that it has become the norm.

Copy link

Choose a reason for hiding this comment

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

Checked, this is not JIT's doing. The C# compiler optimizes away !!.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was mistaken. The C# compiler optimizes away !!. I think this behavior is correct for the reason @mikedn calls out: the C# ! operator is not the same as the C ! operator, and the user-observable side effect of double-notting a bool is to keep the original value. So this code isn't working around a bug as much as it is trying to make up for a missing language feature. I should reword the comment.

Additionally, as far as I can tell the JIT doesn't actually have a concept of a ! operator; the not opcode is instead equivalent to one's complement (~). So putting two not opcodes into the stream doesn't have the desired effect either.

I can't think of a more efficient way of performing this normalization. One option would be to not perform the normalization, but there's probably somebody somewhere who's somehow passing a true value other than 1 and relying on the existing logic.

Copy link

@mikedn mikedn Feb 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Additionally, as far as I can tell the JIT doesn't actually have a concept of a ! operator; the not opcode is instead equivalent to one's complement (~). So putting two not opcodes into the stream doesn't have the desired effect either.

The usual way to negate a 0/1 bool is xor 1. Funnily enough, the C# compilers assumes 0/1 bools but fails to perform this particular optimization.

One option would be to not perform the normalization, but there's probably somebody somewhere who's somehow passing a true value other than 1 and relying on the existing logic.

I doubt that not performing the normalization would have any meaningful impact considering that both the JIT (and AFAIR the JIT does some rather dubious things in this area) and the C# compilers perform various other optimizations assuming 0/1 bools. The ship has sailed a long time ago.

Copy link
Member

Choose a reason for hiding this comment

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

While C# does not clearly define the exact values that true and false are (just that there are only two values).

True. The C# spec does not explicitly define the value of true to any specific numerical value. However the C# compiler (as well as F#, VB, Cobol, etc ... every .NET language in existance) defines the literal true as the value 1 / ldc.i4.1.

Scenarios where the literal true is imagined as any other numeric value are academic.

Since the C# spec doesn't explicitly call out what these values are, but the implementation generally handles it as false is 0; true is not 0

Incorrect. The compiler assumes booleans can only be 1 or 0 and emits it's logic based on that. Sure there are times where C#'s emitted code will happen to also work with a 2+ boolean value but that is just coincidence, not design.

The bool type represents boolean logical quantities. The possible values of type bool are true and false.

Note that the boolean type itself agrees with the C# compiler here on the numeric values of true and false. Both in the constants it defines internally and the values that are returned from TryParse.

Copy link
Member

Choose a reason for hiding this comment

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

Scenarios where the literal true is imagined as any other numeric value are academic.

Potentially academic, but still inline with the IL definition:

A bit pattern with any one or more bits set (analogous to a non-zero integer) denotes a value of true.

I do think it would be desirable, and within the realm of reason, to have the C# spec explicitly list the values it recognizes as true/false; otherwise, two independent language implementations may do it differently (one may do 0/1, like Roslyn; and the other may do 0/not 0, like the IL spec).

Also, while I still think fixing the C# spec to explicitly have the same definition of true (that is, true is a non-zero bit pattern) would be nice. It could also be considered a breaking change in the scenarios that @mikedn listed above, since the code may now execute differently from before.

Copy link
Member

Choose a reason for hiding this comment

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

The C# language specification is not specific to any particular runtime platform. The only observable consequence of placing something like that in the specification would be the behavior you get when you overlay a bool and a byte in unsafe code. Specifying that would be a peculiar departure from the normal situation in which the specification avoids telling you in detail the way unsafe code works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and the same time the C# compile transforms b1 && b2 into a bitwise and. That is only valid for 0/1 bools.

@mikedn I was surprised to see that making this a bitwise operator is a Roslyn-specific behavior. Earlier versions of the C# compiler use short-circuiting logic. So this means technically there has already been a breaking change in the language, though the true number of people impacted is probably minimal. (Of course, there's still wonkiness with the other operators.)

Copy link

Choose a reason for hiding this comment

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

I was surprised to see that making this a bitwise operator is a Roslyn-specific behavior

Note that the JIT does it too. But I'm not sure when it started doing it.

return JitHelpers.BoolToByteNonNormalized(JitHelpers.BoolToByteNonNormalized(value) != 0);
}

public static byte ToByte(byte value)
Expand Down Expand Up @@ -881,7 +885,9 @@ public static short ToInt16(object value, IFormatProvider provider)

public static short ToInt16(bool value)
{
return value ? (short)Boolean.True : (short)Boolean.False;
// Ideally we'd call BoolToByte(!!value) to normalize any true value to 1,
// but JIT optimizes !! away. The pattern below defeats this optimization.
return JitHelpers.BoolToByteNonNormalized(JitHelpers.BoolToByteNonNormalized(value) != 0);
}

public static short ToInt16(char value)
Expand Down Expand Up @@ -995,7 +1001,9 @@ public static ushort ToUInt16(object value, IFormatProvider provider)
[CLSCompliant(false)]
public static ushort ToUInt16(bool value)
{
return value ? (ushort)Boolean.True : (ushort)Boolean.False;
// Ideally we'd call BoolToByte(!!value) to normalize any true value to 1,
// but JIT optimizes !! away. The pattern below defeats this optimization.
return JitHelpers.BoolToByteNonNormalized(JitHelpers.BoolToByteNonNormalized(value) != 0);
}

[CLSCompliant(false)]
Expand Down Expand Up @@ -1117,7 +1125,9 @@ public static int ToInt32(object value, IFormatProvider provider)

public static int ToInt32(bool value)
{
return value ? Boolean.True : Boolean.False;
// Ideally we'd call BoolToByte(!!value) to normalize any true value to 1,
// but JIT optimizes !! away. The pattern below defeats this optimization.
return JitHelpers.BoolToByteNonNormalized(JitHelpers.BoolToByteNonNormalized(value) != 0);
}

public static int ToInt32(char value)
Expand Down Expand Up @@ -1248,7 +1258,9 @@ public static uint ToUInt32(object value, IFormatProvider provider)
[CLSCompliant(false)]
public static uint ToUInt32(bool value)
{
return value ? (uint)Boolean.True : (uint)Boolean.False;
// Ideally we'd call BoolToByte(!!value) to normalize any true value to 1,
// but JIT optimizes !! away. The pattern below defeats this optimization.
return JitHelpers.BoolToByteNonNormalized(JitHelpers.BoolToByteNonNormalized(value) != 0);
}

[CLSCompliant(false)]
Expand Down Expand Up @@ -1375,7 +1387,9 @@ public static long ToInt64(object value, IFormatProvider provider)

public static long ToInt64(bool value)
{
return value ? Boolean.True : Boolean.False;
// Ideally we'd call BoolToByte(!!value) to normalize any true value to 1,
// but JIT optimizes !! away. The pattern below defeats this optimization.
return JitHelpers.BoolToByteNonNormalized(JitHelpers.BoolToByteNonNormalized(value) != 0);
}

public static long ToInt64(char value)
Expand Down Expand Up @@ -1483,7 +1497,9 @@ public static ulong ToUInt64(object value, IFormatProvider provider)
[CLSCompliant(false)]
public static ulong ToUInt64(bool value)
{
return value ? (ulong)Boolean.True : (ulong)Boolean.False;
// Ideally we'd call BoolToByte(!!value) to normalize any true value to 1,
// but JIT optimizes !! away. The pattern below defeats this optimization.
return JitHelpers.BoolToByteNonNormalized(JitHelpers.BoolToByteNonNormalized(value) != 0);
}

[CLSCompliant(false)]
Expand Down
22 changes: 22 additions & 0 deletions src/mscorlib/src/System/Runtime/CompilerServices/jithelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,5 +191,27 @@ static internal ref byte GetRawSzArrayData(this Array array)
typeof(ArrayPinningHelper).ToString(); // Type used by the actual method body
throw new InvalidOperationException();
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
static internal byte BoolToByteNonNormalized(bool value)
Copy link

Choose a reason for hiding this comment

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

Why does this return byte? int would make more sense. A bool is 1 byte in size only when stored in memory (e.g. in a class field or array element). It is never 1 byte on the evaluation stack.

Copy link

Choose a reason for hiding this comment

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

Maybe this method should be added to Unsafe. It basically works around a C# language limitation (like other Unsafe methods do) and at the same time it is slightly unsafe due to the possibility of a bool not being only 0 or 1.

Copy link
Member Author

@GrabYourPitchforks GrabYourPitchforks Feb 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I was hoping that there might be some cases where RyuJIT might elide the movzx instruction if it was kept as an 8-bit integral type all the way throughout the call sites. I'm thinking of cases like myByteArray[idx] = BoolToByte(condition);, where everything would just compile to a single mov byte ptr [rdx + rcx], eax instruction. But I'm not married to returning an 8-bit integral type, and if the JIT is smart enough to make this all work anyway then all the better. :)

Copy link

Choose a reason for hiding this comment

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

I'm thinking of cases like myByteArray[idx] = BoolToByte(condition);, where everything would just compile to a single mov byte ptr [rdx + rcx], eax instruction.

I already sent a PR to fix that specific case. The JIT already eliminates movzx in some other cases.

At the same time, movzx is a cheap instruction (0 latency normally) so it shouldn't have much impact. But if you do see useless movzx being generated, file issues. There are a lot of things that the JIT can't do but there are also quite a few things that the JIT could do, provided someone bothers with writing the necessary code.

{
// The body of this function will be replaced by the EE.

// Per ECMA 335, Sec III.1.1.2, the Boolean data type occupies 1 byte
// of memory. Per Sec. I.8.2.2 and I.8.7, the Boolean data type is internally
// treated as a one-byte integer interchangeable with Byte / SByte. This
// means that "casting" a Boolean to a Byte is both verifiable and free.

// n.b. The return value of this method isn't limited to 0 or 1. It'll contain
// the raw value behind the input Boolean, and technically 'true' could correspond
// to any non-zero value. If the caller doesn't know where the Boolean value
// came from and needs to normalize 'true' to 1, the caller should instead use
// Convert.ToInt32(Boolean), which performs normalization.

// ldarg.0
// ret

throw new InvalidOperationException();
}
}
}
11 changes: 11 additions & 0 deletions src/vm/jitinterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6973,6 +6973,17 @@ bool getILIntrinsicImplementation(MethodDesc * ftn,
methInfo->options = (CorInfoOptions)0;
return true;
}
else if (tk == MscorlibBinder::GetMethod(METHOD__JIT_HELPERS__BOOL_TO_BYTE_NON_NORMALIZED)->GetMemberDef())
{
static BYTE ilcode[] = { CEE_LDARG_0, CEE_RET };

methInfo->ILCode = const_cast<BYTE*>(ilcode);
methInfo->ILCodeSize = sizeof(ilcode);
methInfo->maxStack = 1;
methInfo->EHcount = 0;
methInfo->options = (CorInfoOptions)0;
return true;
}

return false;
}
Expand Down
1 change: 1 addition & 0 deletions src/vm/mscorlib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,7 @@ DEFINE_METHOD(JIT_HELPERS, UNSAFE_ENUM_CAST_LONG, UnsafeEnumCastLong,
DEFINE_METHOD(JIT_HELPERS, UNSAFE_CAST_TO_STACKPTR,UnsafeCastToStackPointer, NoSig)
#endif // _DEBUG
DEFINE_METHOD(JIT_HELPERS, GET_RAW_SZ_ARRAY_DATA, GetRawSzArrayData, NoSig)
DEFINE_METHOD(JIT_HELPERS, BOOL_TO_BYTE_NON_NORMALIZED, BoolToByteNonNormalized, NoSig)

DEFINE_CLASS(UNSAFE, InternalCompilerServices, Unsafe)
DEFINE_METHOD(UNSAFE, AS_POINTER, AsPointer, NoSig)
Expand Down