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

Remove unnecessary endianness dependent logic #104332

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

buyaa-n
Copy link
Member

@buyaa-n buyaa-n commented Jul 2, 2024

Before my PR #102364 the Base64Encoder was populating uint by concatenating encoded 4 bytes (by accounting the endianness)

uint i0 = Unsafe.Add(ref encodingMap, (IntPtr)(i >> 18));
uint i1 = Unsafe.Add(ref encodingMap, (IntPtr)((i >> 12) & 0x3F));
uint i2 = Unsafe.Add(ref encodingMap, (IntPtr)((i >> 6) & 0x3F));
uint i3 = Unsafe.Add(ref encodingMap, (IntPtr)(i & 0x3F));
if (BitConverter.IsLittleEndian)
{
return i0 | (i1 << 8) | (i2 << 16) | (i3 << 24);
}
else
{
return (i0 << 24) | (i1 << 16) | (i2 << 8) | i3;
}

and was writing that uint to the destination buffer:
result = Encode(src, ref encodingMap);
Unsafe.WriteUnaligned(dest, result);

I changed that to directly assign encoded bytes to destination (instead of concatenating them into one uint and write once), where I kept the endianness logic when I should not, endianness has no effect for ASCII characters:
byte i0 = Unsafe.Add(ref encodingMap, (IntPtr)(i >> 18));
byte i1 = Unsafe.Add(ref encodingMap, (IntPtr)((i >> 12) & 0x3F));
byte i2 = Unsafe.Add(ref encodingMap, (IntPtr)((i >> 6) & 0x3F));
byte i3 = Unsafe.Add(ref encodingMap, (IntPtr)(i & 0x3F));
if (BitConverter.IsLittleEndian)
{
destination[0] = i0;
destination[1] = i1;
destination[2] = i2;
destination[3] = i3;
}
else
{
destination[3] = i0;
destination[2] = i1;
destination[1] = i2;
destination[0] = i3;
}

Fixes #104283

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 2, 2024
@buyaa-n buyaa-n requested review from tmds and stephentoub July 2, 2024 21:20
@buyaa-n
Copy link
Member Author

buyaa-n commented Jul 2, 2024

/azp run runtime-community

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@buyaa-n buyaa-n added area-System.Memory and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 2, 2024
Copy link
Contributor

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

@stephentoub
Copy link
Member

I changed that to directly assign encoded bytes to destination (instead of concatenating them into one uint and write once)

Why?

@buyaa-n
Copy link
Member Author

buyaa-n commented Jul 2, 2024

Why?

I think I had build error for Unsafe.WriteUnaligned(dest, result); when changed the dest type to T, though that doesn't seem happen now, is this less performant? I can revert that change NO, I need to keep it

@buyaa-n
Copy link
Member Author

buyaa-n commented Jul 2, 2024

Why?

Now I know why I did this, writing uint to dest only work for byte* dest, but will not work for ushort* dest

@buyaa-n
Copy link
Member Author

buyaa-n commented Jul 2, 2024

/azp run runtime-community

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephentoub
Copy link
Member

stephentoub commented Jul 3, 2024

Why?

Now I know why I did this, writing uint to dest only work for byte* dest, but will not work for ushort* dest

But writing to a ulong would work in that case, no?

@buyaa-n
Copy link
Member Author

buyaa-n commented Jul 3, 2024

But writing to a ulong would work in that case, no?

Did not think about that 😄. Though it looks to me more efficient, no? Compared to doing several |, << operations, I can change then

@buyaa-n
Copy link
Member Author

buyaa-n commented Jul 3, 2024

Though it looks to me more efficient, no?

According to the local perf test, turns out the old approach is bit more performant, reverted to old approach except the case where padding is omitted and not writing 4 bytes to destination

@buyaa-n
Copy link
Member Author

buyaa-n commented Jul 4, 2024

/azp run runtime-community

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@buyaa-n buyaa-n requested a review from stephentoub July 5, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Base64url tests fail on s390x (big endian)
2 participants