Skip to content

Reduce branches in padding adjustment in FromBase64_ComputeResultLength #115022

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

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

Conversation

rameel
Copy link
Contributor

@rameel rameel commented Apr 24, 2025

No description provided.

Comment on lines 2912 to 2913
ReadOnlySpan<byte> map = [0, 2, 1];
padding = map[padding];
Copy link
Member

Choose a reason for hiding this comment

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

This trades branches vs. memory access -- I don't know which approach will be faster and in benchmarks it will be hard to measure, because branch predictor will do a good job, and for the memory access the data will be in the cpu caches, so fast retrieval.

Or put differently: it trades possible branch mispredicts vs. potential cache eviction.

Maybe it's better to write the code as idiomatic as possible and let the compilers optimize it out. Actually with a switch it looks quite good and produces code similar to what native compilers produce.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, writing the code in a more idiomatic style is preferable. However, at the moment, the JIT doesn't seem to generate a value-to-result mapping (like C/C++ does) for simple cases like this. Even for a switch, it just falls back to a jump table, which still involves a memory access.

I agree - it's better to either leave it as-is or rewrite it using a switch, and help the JIT recognize and optimize such patterns. I think I even saw an issue about this a while ago, but couldn't find it right away.

Just for reference, here's what C/C++ produces:

mov     edi, edi
mov     eax, DWORD PTR CSWTCH.1[0+rdi*4]

CSWTCH.1:
        .long   0
        .long   2
        .long   1

As we can see the C/C++ compiler prefers a memory access here, rather than relying on the branch predictor.

And here's what the JIT produces in my case - basically the same thing as the C++ version:

mov      rdi, 0x72BEDC82CB80      ; static handle
movzx    rax, byte  ptr [rax+rdi]

And here's what a typical switch ends up looking like:

lea      rdi, [reloc @RWD00]
mov      edi, dword ptr [rdi+4*rax]
lea      rcx, G_M26941_IG02
add      rdi, rcx
jmp      rdi
mov      eax, 1
jmp      SHORT G_M26941_IG07
mov      eax, 2
jmp      SHORT G_M26941_IG07
xor      eax, eax
G_M26941_IG07:  ;; offset=0x0035

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve found the issue related to this: #114041

Copy link
Member

Choose a reason for hiding this comment

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

So I think it would be still good to change to the switch-approach here.
Once the JIT got improved, then the benefit here comes for free.

@rameel rameel closed this Apr 25, 2025
@rameel rameel reopened this Apr 28, 2025
@am11 am11 added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 28, 2025
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants