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

Improve DefaultCodecRegistry.CacheKey#hashCode() to eliminate Object[] allocation #1988

Open
wants to merge 2 commits into
base: 4.x
Choose a base branch
from

Conversation

tatu-at-datastax
Copy link

@tatu-at-datastax tatu-at-datastax commented Nov 16, 2024

So: while this may seem trivial, profiling showed significant Object[] allocations here (since Object.hashCode(Object... args) takes varargs which gets converted into Object[]) so figured out it's worth improving upon.
Should produce same hash code (although that should not matter greatly as long as hash code works well enough).

@tatu-at-datastax
Copy link
Author

Looks like there is no CI (Github Actions) set up; but local mvn clean test did pass FWTW.

@tatu-at-datastax tatu-at-datastax changed the title Improve DefaultCodecRegisry.CacheKey#hashCode() to eliminate Object[] allocation Improve DefaultCodecRegistry.CacheKey#hashCode() to eliminate Object[] allocation Nov 19, 2024
@tatu-at-datastax
Copy link
Author

Not sure what to do wrt CI, due to simplicity of change guessing there are transient failures.

@absurdfarce Not a big deal but this seems like a simple fix, low-hanging fruit (as per my note was seen at a mem-alloc profiling)

int h2 = Objects.hashCode(javaType);
int h3 = Boolean.hashCode(isJavaCovariant);

return 31 * (31 * h1 + h2) + h3;
Copy link
Contributor

Choose a reason for hiding this comment

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

According to List.hashCode()

     int hashCode = 1;
     for (E e : list)
         hashCode = 31*hashCode + (e==null ? 0 : e.hashCode());

Isn't it ((31+h1)*31+h2)*31+h3?

Copy link
Author

Choose a reason for hiding this comment

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

True. I missed the initial multiplication. Will change.

Copy link
Author

Choose a reason for hiding this comment

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

Changed as suggested.

Copy link
Contributor

@SiyaoIsHiding SiyaoIsHiding left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! Makes sense to me. Another outstanding PR is also about Objects.hash() Object[] allocation performance issues discovered by profiling. It makes me wonder what else Objects.hash() we should fix. We have a lot of Objects.hash() in our codebase.

@tatu-at-datastax
Copy link
Author

Thank you for the fix! Makes sense to me. Another outstanding PR is also about Objects.hash() Object[] allocation performance issues discovered by profiling. It makes me wonder what else Objects.hash() we should fix. We have a lot of Objects.hash() in our codebase.

Interesting & good question. I think most of those cases do not probably matter that much (not in hot path of code flow) so it might be ok to change only cases that show up in profiling?

Then again it might not be bad idea to have a look at how many other cases exist.

@SiyaoIsHiding
Copy link
Contributor

A quick text search of Objects.hash returns 60 matches in our codebase. In that case we probably should just implement our own util.objectsHash.

@tatu-at-datastax
Copy link
Author

@SiyaoIsHiding if that can be done, sure. But since the allocation comes from varargs parameter, it probably requires a few overloads (with 1, 2, 3 field values to hash).

@SiyaoIsHiding
Copy link
Contributor

Filed ticket as CASSJAVA-68.

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