Add methods to manipulate Half in binary by huoyaoyuan · Pull Request #40882 · dotnet/runtime

huoyaoyuan · 2020-08-15T13:02:42Z

Closes #38456 . Closes #38288 .

Dotnet-GitSync-Bot · 2020-08-15T13:02:45Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2020-08-15T15:28:07Z

Tagging subscribers to this area: @tannergooding, @pgovind
See info in area-owners.md if you want to be subscribed.

src/libraries/System.Private.CoreLib/src/System/IO/BinaryWriter.cs

huoyaoyuan · 2020-08-25T10:12:21Z

Merging master fixes half write tests. The new failing tests looks like unrelated.

jeffhandley · 2020-08-26T04:19:37Z

@jkotas I do not think this would meet the bar for merging into 5.0; do you agree? If so, I'll change #38456 to be 6.0.0.

huoyaoyuan · 2020-08-26T04:50:04Z

I'm OK with 6.0. This should be only a subset of operations for Half.

tannergooding · 2020-08-27T16:21:19Z

@GrabYourPitchforks, @adamsitnik, could you give this a second review since you own System.Memory?

adamsitnik

LGTM, I've left some comments, but mostly nits

src/libraries/System.Private.CoreLib/src/System/Buffers/Binary/ReaderBigEndian.cs

adamsitnik · 2020-08-31T16:55:35Z

src/libraries/System.Private.CoreLib/src/System/BitConverter.cs


        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal static unsafe short HalfToInt16Bits(Half value)
+        public static unsafe short HalfToInt16Bits(Half value)


not related to your changes: I would be really surprised if this method was not inlined without [MethodImpl(MethodImplOptions.AggressiveInlining)] applied

src/libraries/System.Private.CoreLib/src/System/Buffers/Binary/ReaderLittleEndian.cs

adamsitnik · 2020-08-31T17:01:13Z

src/libraries/System.Private.CoreLib/src/System/Buffers/Binary/ReaderLittleEndian.cs

+            return !BitConverter.IsLittleEndian ?
+                BitConverter.Int16BitsToHalf(ReverseEndianness(MemoryMarshal.Read<short>(source))) :
+                MemoryMarshal.Read<Half>(source);


pedantic nit: removing extra ! can simplify the code?

Suggested change

return !BitConverter.IsLittleEndian ?

BitConverter.Int16BitsToHalf(ReverseEndianness(MemoryMarshal.Read<short>(source))) :

MemoryMarshal.Read<Half>(source);

return BitConverter.IsLittleEndian ?

MemoryMarshal.Read<Half>(source) :

BitConverter.Int16BitsToHalf(ReverseEndianness(MemoryMarshal.Read<short>(source)));

I just copied the code body from double overload and changed the types. They should be consistent after this PR right?

adamsitnik · 2020-08-31T17:11:35Z

src/libraries/System.Private.CoreLib/src/System/Buffers/Binary/WriterBigEndian.cs

+            if (BitConverter.IsLittleEndian)
+            {
+                short tmp = ReverseEndianness(BitConverter.HalfToInt16Bits(value));
+                MemoryMarshal.Write(destination, ref tmp);
+            }
+            else
+            {
+                MemoryMarshal.Write(destination, ref value);
+            }


very-very pedantic nit: you are sometimes using else block, sometimes not`

if (condition) { // do one thing // return sth; } // return sth else

if (condition) { // do one thing // return sth; } else { // return sth else }

it would be good to stick to one pattern and follow it in the entire PR

adamsitnik · 2020-08-31T17:16:30Z

src/libraries/System.Private.CoreLib/src/System/IO/BinaryWriter.cs

-            _buffer[1] = (byte)(TmpValue >> 8);
-            _buffer[2] = (byte)(TmpValue >> 16);
-            _buffer[3] = (byte)(TmpValue >> 24);
+            BinaryPrimitives.WriteSingleLittleEndian(_buffer, value);


since you have changed it for single overload, should we do the same for int, uint, short, ushort and other methods that don't use BinaryPrimitives? (just for consistency)

runtime/src/libraries/System.Private.CoreLib/src/System/IO/BinaryWriter.cs

Lines 242 to 283 in 88cf0ed

public virtual void Write(short value)

{

_buffer[0] = (byte)value;

_buffer[1] = (byte)(value >> 8);

OutStream.Write(_buffer, 0, 2);

}

// Writes a two-byte unsigned integer to this stream. The current position

// of the stream is advanced by two.

//

[CLSCompliant(false)]

public virtual void Write(ushort value)

{

_buffer[0] = (byte)value;

_buffer[1] = (byte)(value >> 8);

OutStream.Write(_buffer, 0, 2);

}

// Writes a four-byte signed integer to this stream. The current position

// of the stream is advanced by four.

//

public virtual void Write(int value)

{

_buffer[0] = (byte)value;

_buffer[1] = (byte)(value >> 8);

_buffer[2] = (byte)(value >> 16);

_buffer[3] = (byte)(value >> 24);

OutStream.Write(_buffer, 0, 4);

}

// Writes a four-byte unsigned integer to this stream. The current position

// of the stream is advanced by four.

//

[CLSCompliant(false)]

public virtual void Write(uint value)

{

_buffer[0] = (byte)value;

_buffer[1] = (byte)(value >> 8);

_buffer[2] = (byte)(value >> 16);

_buffer[3] = (byte)(value >> 24);

OutStream.Write(_buffer, 0, 4);

}

This was explored in #37582 (comment) . Using the Span-based helpers for <8 byte types is not profitable (converting array to Span is cheap, but it is not free).

Using the Span-based helpers for <8 byte types is not profitable

@jkotas thanks for another valuable comment! Does it mean that the single change should be reverted and the new Write(Half value) method should not be using BinaryPrimitives as well?

I would think so, unless we have data that suggest otherwise.

src/libraries/System.Runtime/tests/System/HalfTests.cs

jkotas · 2020-09-01T20:17:57Z

huoyaoyuan Thank you!

huoyaoyuan added 6 commits August 15, 2020 19:16

Expose HalfToInt16Bits and Int16BitsToHalf.

428d797

Use HalfToInt16Bits and Int16BitsToHalf in tests.

873fe9a

Add and expose BinaryPrimitive Half overloads.

42bde82

Test BinaryPrimitive Half overloads.

d29e8ec

Add and expose BinaryReader/Writer Half overloads.

b7d3507

Test BinaryReader/Writer Half overloads.

8632f62

Dotnet-GitSync-Bot added area-Interop-coreclr new-api-needs-documentation labels Aug 15, 2020

jkotas added the area-System.Numerics label Aug 15, 2020

jkotas removed the area-Interop-coreclr label Aug 15, 2020

jkotas reviewed Aug 15, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/BinaryWriter.cs Outdated Show resolved Hide resolved

Don't call other virtual methods in BinaryWriter.Write.

a1e9080

huoyaoyuan mentioned this pull request Aug 24, 2020

Semantics of Equals is different for System.Half comparing to other floating point types #41258

Closed

Merge branch 'master' into half-binary

88cf0ed

jeffhandley mentioned this pull request Aug 26, 2020

Consider adding Half support to the BinaryPrimitives class #38456

Closed

tannergooding approved these changes Aug 27, 2020

View reviewed changes

adamsitnik reviewed Aug 31, 2020

View reviewed changes

Don't use Span-based overload for <4 bytes fp types.

14e7023

jkotas approved these changes Sep 1, 2020

View reviewed changes

jkotas reviewed Sep 1, 2020

View reviewed changes

src/libraries/System.Runtime/tests/System/HalfTests.cs Outdated Show resolved Hide resolved

jkotas reviewed Sep 1, 2020

View reviewed changes

src/libraries/System.Runtime/tests/System/HalfTests.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

952eba4

jkotas merged commit 393c1b2 into dotnet:master Sep 1, 2020

huoyaoyuan deleted the half-binary branch September 1, 2020 20:21

runfoapp bot mentioned this pull request Sep 29, 2020

Test initial triage jaredpar/runfo#50

Closed

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

	public virtual void Write(short value)
	{
	_buffer[0] = (byte)value;
	_buffer[1] = (byte)(value >> 8);
	OutStream.Write(_buffer, 0, 2);
	}

	// Writes a two-byte unsigned integer to this stream. The current position
	// of the stream is advanced by two.
	//
	[CLSCompliant(false)]
	public virtual void Write(ushort value)
	{
	_buffer[0] = (byte)value;
	_buffer[1] = (byte)(value >> 8);
	OutStream.Write(_buffer, 0, 2);
	}

	// Writes a four-byte signed integer to this stream. The current position
	// of the stream is advanced by four.
	//
	public virtual void Write(int value)
	{
	_buffer[0] = (byte)value;
	_buffer[1] = (byte)(value >> 8);
	_buffer[2] = (byte)(value >> 16);
	_buffer[3] = (byte)(value >> 24);
	OutStream.Write(_buffer, 0, 4);
	}

	// Writes a four-byte unsigned integer to this stream. The current position
	// of the stream is advanced by four.
	//
	[CLSCompliant(false)]
	public virtual void Write(uint value)
	{
	_buffer[0] = (byte)value;
	_buffer[1] = (byte)(value >> 8);
	_buffer[2] = (byte)(value >> 16);
	_buffer[3] = (byte)(value >> 24);
	OutStream.Write(_buffer, 0, 4);
	}

Conversation

huoyaoyuan commented Aug 15, 2020

Uh oh!

Dotnet-GitSync-Bot commented Aug 15, 2020

Uh oh!

ghost commented Aug 15, 2020

Uh oh!

Uh oh!

huoyaoyuan commented Aug 25, 2020

Uh oh!

jeffhandley commented Aug 26, 2020

Uh oh!

huoyaoyuan commented Aug 26, 2020

Uh oh!

tannergooding commented Aug 27, 2020

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkotas commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants