Skip to content

refactor: replace 3rd-party xxhash impl with in-house reimpl #2310

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

Merged
merged 2 commits into from
Nov 15, 2022

Conversation

0xFA11
Copy link
Contributor

@0xFA11 0xFA11 commented Nov 15, 2022

This PR replaces 3rd-party xxHash32 & xxHash64 implementations with in-house full reimplementation.

Changelog

  • Changed 3rd-party XXHash (32 & 64) implementation with an in-house reimplementation

Testing and Documentation

  • Added XXHashTests unit tests

@0xFA11 0xFA11 requested a review from a team as a code owner November 15, 2022 02:25

namespace Unity.Netcode.EditorTests
{
public class XXHashTests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We never had unit tests to verify hash results, I implemented them with some hardcoded strings and expected hardcoded results.

Having said that, even though it's a full reimplementation of the 3rd-party one, they both give us the exact same results as expected as they are both completely valid and accurate implementations — we're just getting rid of our external dependency and also making them just slightly more performant/efficient.

@0xFA11 0xFA11 enabled auto-merge (squash) November 15, 2022 02:30
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

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

Looks good.
👍

@0xFA11 0xFA11 merged commit f1870cd into develop Nov 15, 2022
@0xFA11 0xFA11 deleted the refactor/xxhash branch November 15, 2022 19:22
jakobbbb pushed a commit to GooseGirlGames/com.unity.netcode.gameobjects that referenced this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants