datastax
Improve DefaultCodecRegistry.CacheKey#hashCode() to eliminate Object[] allocation
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).
Looks like there is no CI (Github Actions) set up; but local mvn clean test did pass FWTW.
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)
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 elseObjects.hash()we should fix. We have a lot ofObjects.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.
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.
@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).
Apologies for the delay here, gang; been too involved in other things. Jenkins run kicked off for this just to ensure there aren't any regressions from this but I can't imagine that there would be.
I double checked the issue and can confirm that it is present. I have used the following test code to validate it:
public static void main(String[] args) throws ExecutionException, InterruptedException {
try (CqlSession cqlSession = CqlSession.builder()
.addContactPoint(new InetSocketAddress("localhost", 9042))
.withLocalDatacenter("datacenter1")
.build()) {
CodecRegistry codecRegistry = cqlSession.getContext().getCodecRegistry();
GenericType<List<String>> listOfStringType = GenericType.listOf(String.class);
int result = 0;
for (int i = 0; i < 500_000_000; i++) {
TypeCodec<List<String>> codec = codecRegistry.codecFor(listOfStringType);
result ^= codec.hashCode(); // just to avoid a possible dead code elimination
}
System.out.println(result);
}
}
}
and run it using OpenJDK Runtime Environment Temurin-11.0.15+10 (build 11.0.15+10) with the following VM options to capture a JFR recording:
-XX:StartFlightRecording=duration=200s,filename=flight.jfr,settings=profile
Result:
The allocations are present along the whole time during the test. Originally I hoped that JVM escape analysis (https://www.javaspecialists.eu/archive/Issue179-Escape-Analysis.html) should eliminate such allocations once the logic was compiled by C2 JIT compiler but it sees does not work in this case for this OpenJDK version. https://jpbempel.github.io/2020/08/02/when-escape-analysis-fails-you.html
So, the optimization is reasonable from my point of view. Note: CacheKey is used when we lookup a codec not for a primitive type or explicitly registered, so the result of the optimization is observable only for workloads when we work with collection/UDT columns.
From code review point of view - it is ok for me as well. +1, thank you for reporting it.
@tatu-at-datastax can you squash all this down to a single commit so that we can get this guy merged?
Regarding hash value caching in CacheKey: it is created each time to access the cache + within LoadingCache hash values are cached for already stored entities in com.datastax.oss.driver.shaded.guava.common.cache.LocalCache.StrongEntry#hash, so I do not see benefit to pre-compute this value, it will only increase CacheKey object size.
@netudima Excellent. On this:
Note: CacheKey is used when we lookup a codec not for a primitive type or explicitly registered, so the result of the optimization is observable only for workloads when we work with collection/UDT columns.
we specifically have such use case for service we have -- data model depends heavily on maps, sets and lists. Makes sense it hasn't been observable for other users then since this is probably not all that common use case.
Many thanks @tatu-at-datastax! We're all set to go with this one!
Great! Thank you all for guiding me through & doing thorough verification.