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

[API Proposal]: BinaryPrimitives.ReverseEndianness #75901

Closed
stephentoub opened this issue Sep 20, 2022 · 4 comments · Fixed by #76339
Closed

[API Proposal]: BinaryPrimitives.ReverseEndianness #75901

stephentoub opened this issue Sep 20, 2022 · 4 comments · Fixed by #76339
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Sep 20, 2022

Background and motivation

BinaryPrimitives today exposes a ReverseEndianness method for byte, ushort, uint, ulong, sbyte, short, int, and long, and #72107 proposes adding overloads for nuint, UInt128, nint, and Int128 as well. However, we lack versions of this API for processing multiple elements at a time. A developer can write this in their own loop, e.g.

but it'd be nice if a) they didn't have to, and b) it could be vectorized (for at least some data types).

API Proposal

namespace System.Buffers.Binary;

public static class BinaryPrimitives
{
+    public static void ReverseEndianness(ReadOnlySpan<ushort> source, Span<ushort> destination);
+    public static void ReverseEndianness(ReadOnlySpan<short> source, Span<short> destination);
+    public static void ReverseEndianness(ReadOnlySpan<uint> source, Span<uint> destination);
+    public static void ReverseEndianness(ReadOnlySpan<int> source, Span<int> destination);
+    public static void ReverseEndianness(ReadOnlySpan<ulong> source, Span<ulong> destination);
+    public static void ReverseEndianness(ReadOnlySpan<long> source, Span<long> destination);
+    public static void ReverseEndianness(ReadOnlySpan<nuint> source, Span<nuint> destination);
+    public static void ReverseEndianness(ReadOnlySpan<nint> source, Span<nint> destination);
+    public static void ReverseEndianness(ReadOnlySpan<UInt128> source, Span<UInt128> destination);
+    public static void ReverseEndianness(ReadOnlySpan<Int128> source, Span<Int128> destination);
}

source and destination may be the same.

I did not include overloads for byte/sbyte as they're useless (the current byte/sbyte overloads are useless, too).

API Usage

Span<uint> values = ...;
BinaryPrimitives.ReverseEndianness(values);

Alternative Designs

  • Instead of taking a source and a destination span, each overload could instead just take a Span<T> that's reversed in-place, and if you need the results in a different location, you first copy and then reverse in-place in the destination.

Risks

No response

@stephentoub stephentoub added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 20, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 20, 2022
@ghost
Copy link

ghost commented Sep 20, 2022

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

BinaryPrimitives today exposes a ReverseEndianness method for byte, ushort, uint, ulong, sbyte, short, int, and long, and #72107 proposes adding overloads for nuint, UInt128, nint, and Int128 as well. However, we lack versions of this API for processing multiple elements at a time. A developer can write this in their own loop, e.g.

for (int i = 0; i < 16; i++)
{
pTableName[i] = (char)BinaryPrimitives.ReverseEndianness((ushort)pTableName[i]);
}
ushort *pVersion = &p->Version;
for (int i = 0; i < 4; i++)
{
pVersion[i] = BinaryPrimitives.ReverseEndianness(pVersion[i]);
}

for (int i = 0; i < 16; i++)
{
pCodePageName[i] = (char)BinaryPrimitives.ReverseEndianness((ushort)pCodePageName[i]);
}

https://github.com/dorssel/usbipd-win/blob/2f7cbb732889ed00617e85f2f0b22239e8533960/Usbipd/Interop/UsbIp.cs#L165-L171
https://github.com/HaveIBeenPwned/PwnedPasswordsAzureFunction/blob/0f81f2258f8f1f8f8486adf0820b141a43be396e/Shared/HaveIBeenPwned.PwnedPasswords.Shared/MD4.cs#L51-L55
https://github.com/ratbuddy/ratbuddyssey/blob/65166ec837abc2ad78e2b2f239e8b1eb26aaaeef/Ratbuddyssey/AudysseyMultEQAvrTcpClientWithTimeout.cs#L51-L55
https://github.com/saucecontrol/PhotoSauce/blob/d2555900c980c38a521e6381fc9b7a52dea66e3c/src/MagicScaler/Metadata/ExifReader.cs#L72-L73
https://github.com/mono/mono/blob/b40e9939a7d07b30a75625692874f02bcc9be18f/mcs/class/referencesource/mscorlib/system/io/binaryreader.cs#L249-L250
https://github.com/couchbase/couchbase-net-client/blob/4a755f449d2c4b37a4a44ba7107f2ba7a8594d80/src/Couchbase/Core/IO/Operations/Hello.cs#L59-L63
https://github.com/Nenkai/PDTools/blob/2a9a5963f3754a029dfe0287aaddf8cbd804c763/PDTools.Files/Textures/TextureSet3.cs#L67-L68
https://github.com/Nenkai/GTToolsSharp/blob/c33dbdb53e905be5b4b31a6575595749d5533a73/GTToolsSharp/Encryption/GT5POldCrypto.cs#L51-L52
#72107 (comment)

but it'd be nice if a) they didn't have to, and b) it could be vectorized.

API Proposal

namespace System.Buffers.Binary;

public static class BinaryPrimitives
{
+    public static void ReverseEndianness(ReadOnlySpan<ushort> source, Span<ushort> destination);
+    public static void ReverseEndianness(ReadOnlySpan<ushort> source, Span<ushort> destination);
+    public static void ReverseEndianness(ReadOnlySpan<ushort> source, Span<ushort> destination);
+    public static void ReverseEndianness(ReadOnlySpan<ushort> source, Span<ushort> destination);
+    public static void ReverseEndianness(ReadOnlySpan<ushort> source, Span<ushort> destination);
+    public static void ReverseEndianness(ReadOnlySpan<ushort> source, Span<ushort> destination);
+    public static void ReverseEndianness(ReadOnlySpan<ushort> source, Span<ushort> destination);+    public static void ReverseEndianness(ReadOnlySpan<ushort> source, Span<ushort> destination);}

API Usage

// Fancy the value
var c = new MyFancyCollection<int>();
c.Fancy(42);

// Getting the values out
foreach (var v in c)
    Console.WriteLine(v);

Alternative Designs

No response

Risks

No response

Author: stephentoub
Assignees: -
Labels:

api-suggestion, area-System.Memory

Milestone: -

@stephentoub stephentoub changed the title [API Proposal]: BinaryPrimitives [API Proposal]: BinaryPrimitives.ReverseEndianness Sep 20, 2022
@stephentoub
Copy link
Member Author

Duplicate of #75900

I closed the other one... somehow it got opened twice.

@stephentoub
Copy link
Member Author

stephentoub commented Sep 20, 2022

What's the anticipated behavior around in-place reversals?

That it works, i.e. that it's semantically equivalent to:

for (int i = 0; i < source.Length; i++)
{
    destination[i] = BinaryPrimitives.ReverseEndianness(source[i]);
}

where, as noted in the issue, source and destination may be the same.

There's the question of what if source and destination overlap but don't have the same starting ref; I don't have a strong opinion about that case, since none of the examples I've seen have that. Every case I've seen is either using the exact same span/array as input/output, or copying to an entirely separate array/span and reversing along the way.

If we were really concerned about it, we could simplify this down to just taking a single Span<T> rather than two spans, and if you want to copy and reverse, you first copy and then reverse.

@stephentoub stephentoub added this to the 8.0.0 milestone Sep 20, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Sep 20, 2022
@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Sep 27, 2022
@terrajobst
Copy link
Member

terrajobst commented Sep 27, 2022

Video

  • Looks good as proposed
    • We discussed offering a single argument one. One argument against it that it might make code harder to read because ignoring the return value for scalars would be wrong. Another argument against it is that the two argument version is more versatile (it can handle passing the same span).
namespace System.Buffers.Binary;

public static class BinaryPrimitives
{
    public static void ReverseEndianness(ReadOnlySpan<ushort> source, Span<ushort> destination);
    public static void ReverseEndianness(ReadOnlySpan<short> source, Span<short> destination);
    public static void ReverseEndianness(ReadOnlySpan<uint> source, Span<uint> destination);
    public static void ReverseEndianness(ReadOnlySpan<int> source, Span<int> destination);
    public static void ReverseEndianness(ReadOnlySpan<ulong> source, Span<ulong> destination);
    public static void ReverseEndianness(ReadOnlySpan<long> source, Span<long> destination);
    public static void ReverseEndianness(ReadOnlySpan<nuint> source, Span<nuint> destination);
    public static void ReverseEndianness(ReadOnlySpan<nint> source, Span<nint> destination);
    public static void ReverseEndianness(ReadOnlySpan<UInt128> source, Span<UInt128> destination);
    public static void ReverseEndianness(ReadOnlySpan<Int128> source, Span<Int128> destination);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 27, 2022
@stephentoub stephentoub self-assigned this Sep 28, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 28, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 4, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 2022
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.Memory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants