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

Update FarmHashFingerprint64.java #7549

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gopinath-vasalamarri
Copy link

Remove the data dependency between loads and both the rotates.

We can rewrite the code in such a way that rotates can start computing as soon as the required loads are present instead of waiting for both the loads to retire.

Remove the data dependency between loads and both the rotates.  

We can rewrite the code in such a way that rotates can start computing as soon as the required loads are present instead of waiting for both the loads to retire.
Copy link

google-cla bot commented Dec 13, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@cpovirk
Copy link
Member

cpovirk commented Dec 13, 2024

This might be a good improvement, but our existing (internal) benchmarks are kind of questionable, and our fleet-wide profiling shows a negligible amount of CPU in these functions for us, so we are unlikely to improve the benchmarks enough to see if this would help. If you want to try to put something together with JMH, you could, but I'm not even sure I'd guarantee that we'd end up merging this, given that it makes the implementation code different from the C++ implementation. (Now, if you could get the C++ implementation updated to match this because this change showed benefits there, then we might follow.)

@cpovirk cpovirk added type=enhancement Make an existing feature better package=hash P4 labels Dec 13, 2024
@gopinath-vasalamarri
Copy link
Author

Thank you Chris for the suggestions.

One meta question on the comment -- is there a list somewhere, of functions in Guava, which consume the highest amount of CPU ?.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 package=hash type=enhancement Make an existing feature better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants