-
Notifications
You must be signed in to change notification settings - Fork 79
[CORE] Refactor XferCRC to make it branchless and to remove winsock dependency #1228
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
base: main
Are you sure you want to change the base?
Conversation
|
||
UnsignedInt val = 0; | ||
const unsigned char *c = (const unsigned char *)uintPtr; | ||
for (i=0; i<leftover; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could try a switch statement to unroll the loop, though it'd have to be measured to see whether it's an improvement:
switch(leftover)
{
case 3:
val += (c[2] << (2 * 8));
case 2:
val += (c[1] << (1 * 8));
case 1:
val += (c[0] << (0 * 8));
default:
break;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check this out, but if it works that at least gets rid of the second loop.
Perhaps we could go for even fewer branches by using the data size as a template parameter. Most of the call sites use a size that's known at compile-time. |
Replacing the winsock call is good. Removing the hibit branch also makes sense, because the branch predictor likely does not work well there (50% chance). I would like to see a performance comparison when this works without any remaining logical mismatch. |
Fixed the logic on validity handling of the leftover bytes. |
This PR refactors the XferCRC to make the code branchless and to remove the winsock dependency within it.
There may be minimal performance improvement from this as other factors will need changing in the data being passed to the CRC to further improve the performance.
The winsock dependency has been replaced with the endian compat library instead.