Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Single and Double overloads to BinaryPrimitives #2365

Closed
eerhardt opened this issue Mar 5, 2019 · 10 comments · Fixed by #6864
Closed

Add Single and Double overloads to BinaryPrimitives #2365

eerhardt opened this issue Mar 5, 2019 · 10 comments · Fixed by #6864
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime

Comments

@eerhardt
Copy link
Member

eerhardt commented Mar 5, 2019

We currently have overloads to read short, int, long, etc for big and little endian. However, we don't have overloads for float and double. We should add them to complete the types that are supported by this class.

namespace System.Buffers.Binary
{
    public static class BinaryPrimitives
    {
        public static float ReadSingleBigEndian(ReadOnlySpan<byte> buffer);
        public static float ReadSingleLittleEndian(ReadOnlySpan<byte> buffer);
        public static double ReadDoubleBigEndian(ReadOnlySpan<byte> buffer);
        public static double ReadDoubleLittleEndian(ReadOnlySpan<byte> buffer);

        public static bool TryReadSingleBigEndian(ReadOnlySpan<byte> buffer, out float value);
        public static bool TryReadSingleLittleEndian(ReadOnlySpan<byte> buffer, out float value);
        public static bool TryReadDoubleBigEndian(ReadOnlySpan<byte> buffer, out double value);
        public static bool TryReadDoubleLittleEndian(ReadOnlySpan<byte> buffer, out double value);

        public static bool TryWriteSingleBigEndian(System.Span<byte> destination, float value);
        public static bool TryWriteSingleLittleEndian(System.Span<byte> destination, float value);
        public static bool TryWriteDoubleBigEndian(System.Span<byte> destination, double value);
        public static bool TryWriteDoubleLittleEndian(System.Span<byte> destination, double value);

        public static void WriteSingleBigEndian(System.Span<byte> destination, float value);
        public static void WriteSingleLittleEndian(System.Span<byte> destination, float value);
        public static void WriteDoubleBigEndian(System.Span<byte> destination, double value);
        public static void WriteDoubleLittleEndian(System.Span<byte> destination, double value);
    }
}

This API is useful when you are reading/writing to a memory stream that you know is little or big endian. For example, it would have been useful in the following code in the C# implementation of Google FlatBuffers:

Write float/double LittleEndian
https://github.com/google/flatbuffers/blob/1c7d91cc55a9deb05e7ea93ba10b5ab511d29238/net/FlatBuffers/ByteBuffer.cs#L509-L547

Read float/double LittleEndian
https://github.com/google/flatbuffers/blob/1c7d91cc55a9deb05e7ea93ba10b5ab511d29238/net/FlatBuffers/ByteBuffer.cs#L710-L750

A possible workaround is to make 2 calls like the following:

return BitConverter.Int64BitsToDouble(BinaryPrimitives.ReadInt64LittleEndian(buffer));

@stephentoub @jkotas @ahsonkhan @GrabYourPitchforks @tannergooding

@tannergooding
Copy link
Member

We should also consider System.Decimal here. CC. @pentp

@GrabYourPitchforks
Copy link
Member

What does it mean to write decimal in big-endian / little-endian / machine-endian format?

@pentp
Copy link
Contributor

pentp commented Mar 16, 2019

Decimal uses 4 ints internally so its in-memory layout depends on endianess. All the existing (de)serialization methods on decimal/BinaryReader/BinaryWriter use the little-endian memory representation.

While initially it felt like this would be the best place for decimal<->Span APIs also, it looks like the original proposal in dotnet/corefx#35877 might actually be better for API discoverability and there is probably no need for big-endian decimal encodings.

@eerhardt
Copy link
Member Author

eerhardt commented Nov 1, 2019

Marking this as ready for review in order to move this proposal forward.

@terrajobst
Copy link
Member

terrajobst commented Dec 10, 2019

Video

  • Looks good as proposed
  • Should we also add these methods for parity?
    • ReverseEndianness(float)
    • ReverseEndianness(double)
namespace System.Buffers.Binary
{
    public static class BinaryPrimitives
    {
        public static float ReadSingleBigEndian(ReadOnlySpan<byte> buffer);
        public static float ReadSingleLittleEndian(ReadOnlySpan<byte> buffer);
        public static double ReadDoubleBigEndian(ReadOnlySpan<byte> buffer);
        public static double ReadDoubleLittleEndian(ReadOnlySpan<byte> buffer);

        public static bool TryReadSingleBigEndian(ReadOnlySpan<byte> buffer, out float value);
        public static bool TryReadSingleLittleEndian(ReadOnlySpan<byte> buffer, out float value);
        public static bool TryReadDoubleBigEndian(ReadOnlySpan<byte> buffer, out double value);
        public static bool TryReadDoubleLittleEndian(ReadOnlySpan<byte> buffer, out double value);

        public static bool TryWriteSingleBigEndian(System.Span<byte> destination, float value);
        public static bool TryWriteSingleLittleEndian(System.Span<byte> destination, float value);
        public static bool TryWriteDoubleBigEndian(System.Span<byte> destination, double value);
        public static bool TryWriteDoubleLittleEndian(System.Span<byte> destination, double value);

        public static void WriteSingleBigEndian(System.Span<byte> destination, float value);
        public static void WriteSingleLittleEndian(System.Span<byte> destination, float value);
        public static void WriteDoubleBigEndian(System.Span<byte> destination, double value);
        public static void WriteDoubleLittleEndian(System.Span<byte> destination, double value);
    }
}

@jkotas
Copy link
Member

jkotas commented Dec 10, 2019

ReverseEndianness(float)
ReverseEndianness(double)

These do not make sense. They would produce completely bogus combination of bits that do not look anything like a valid floating point number (different from the integer overloads).

@tannergooding
Copy link
Member

I believe the purpose of said APIs is to workaround limitations in other APIs. That is, not every API exposes the underlying Span<T>/ReadOnlySpan<T> and not every API exposes Read/Write methods that take endianness into account.

ReverseEndianness then lets you efficiently fixup the endianness to be a valid set of floating-point bits.

@jkotas
Copy link
Member

jkotas commented Dec 11, 2019

You can always use the longer form: BitConverter.Int64BitsToDouble(BinaryPrimitives.ReadInt64LittleEndian(buffer))

Storing random bit pattern into a floating point register:

  • has large performance penalty on some architectures, e.g. Intel x87 stack is known to have this problem.
  • has risk of unexpected normalization that corrupts the value. (I am not sure whether the ECMA spec would allow it or which current platforms would be exposed to this problem.)

@ahsonkhan
Copy link
Member

For consistency with existing APIs on BinaryPrimitives, the span parameter for the {Try}Read* APIs should be called source and not buffer. Just an FYI for whoever picks this issue up. The {Try}Write* APIs are correct:

-public static float ReadSingleBigEndian(ReadOnlySpan<byte> buffer);
-public static float ReadSingleLittleEndian(ReadOnlySpan<byte> buffer);
-public static double ReadDoubleBigEndian(ReadOnlySpan<byte> buffer);
-public static double ReadDoubleLittleEndian(ReadOnlySpan<byte> buffer);

+public static float ReadSingleBigEndian(ReadOnlySpan<byte> source);
+public static float ReadSingleLittleEndian(ReadOnlySpan<byte> source);
+public static double ReadDoubleBigEndian(ReadOnlySpan<byte> source);
+public static double ReadDoubleLittleEndian(ReadOnlySpan<byte> source);

-public static bool TryReadSingleBigEndian(ReadOnlySpan<byte> buffer, out float value);
-public static bool TryReadSingleLittleEndian(ReadOnlySpan<byte> buffer, out float value);
-public static bool TryReadDoubleBigEndian(ReadOnlySpan<byte> buffer, out double value);
-public static bool TryReadDoubleLittleEndian(ReadOnlySpan<byte> buffer, out double value);

+public static bool TryReadSingleBigEndian(ReadOnlySpan<byte> source, out float value);
+public static bool TryReadSingleLittleEndian(ReadOnlySpan<byte> source, out float value);
+public static bool TryReadDoubleBigEndian(ReadOnlySpan<byte> source, out double value);
+public static bool TryReadDoubleLittleEndian(ReadOnlySpan<byte> source, out double value);

@eerhardt eerhardt self-assigned this Jan 28, 2020
@danmoseley danmoseley transferred this issue from dotnet/corefx Jan 29, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Runtime untriaged New issue has not been triaged by the area owner labels Jan 29, 2020
@danmoseley danmoseley added the api-approved API was approved in API review, it can be implemented label Jan 29, 2020
@danmoseley
Copy link
Member

re-adding api approved label

@danmoseley danmoseley removed the untriaged New issue has not been triaged by the area owner label Jan 29, 2020
eerhardt added a commit to eerhardt/runtime that referenced this issue Jan 31, 2020
eerhardt added a commit that referenced this issue Feb 3, 2020
* Add Single and Double overloads to BinaryPrimitives

Fix #2365
@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants