JAVA-1308: CodecRegistry performance improvements#757
Conversation
gnoremac
commented
Sep 28, 2016
- Separate the built-in codecs cache so it can be backed by a static Map instead of Guava Cache
|
Hi @gnoremac, thanks for your contribution! In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. Sincerely, |
|
Thank you @gnoremac for signing the Contribution License Agreement. Cheers, |
|
for an apples-to-apples comparison, here is a small benchmark comparing this branch against 3.x: Benchmark code here. So it seems that it does bring a performance boost for common cases (cache hits for built-in codec lookups), while showing a slight degradation for other cases, as expected (probably due to the double lookup). This benchmark does not cover on-the-fly codec creation, but I assume the results will be slightly worse for this branch compared to 3.x, due to the double lookup before attempting to create the missing codec. I tend to think that it is a good idea to favor common lookup scenarios in detriment of complex lookups involving on-the-fly codec generation so this improvement seems to go in the good direction. Thanks @gnoremac ! |
|
Sorry I wasn't able to get performance fix for Guava's cache in during its development, and was unsuccessful thereafter. If you move to JDK8 then the rewrite is much faster. But regardless this seems like a good idea. |
|
@ben-manes would you mind rebasing this on top of current 3.x? Otherwise I can take care of it. |
|
I think you meant to ping @gnoremac for the rebase |
|
Created JAVA-1396 to explore Caffeine. |
- Separate the built-in codecs cache so it can be backed by a static Map instead of Guava Cache
| put(TypeCodec.time().getCqlType(), TypeCodec.time()); | ||
| put(TypeCodec.uuid().getCqlType(), TypeCodec.uuid()); // must be declared before TimeUUIDCodec so it gets chosen when CQL type not available | ||
| put(TypeCodec.timeUUID().getCqlType(), TypeCodec.timeUUID()); | ||
| put(TypeCodec.inet().getCqlType(), TypeCodec.inet()); |
There was a problem hiding this comment.
With the recent merge of JAVA-1347, this should have duration as well.
| */ | ||
| public CodecRegistry() { | ||
| this.codecs = new CopyOnWriteArrayList<TypeCodec<?>>(PRIMITIVE_CODECS); | ||
| this.codecs = new CopyOnWriteArrayList<TypeCodec<?>>(); |
There was a problem hiding this comment.
Putting this here because I can't comment on unmodified lines: the doc of the codecs field (line 297) is now outdated:
/**
* The list of registered codecs.
* This list is initialized with the built-in codecs;
* User-defined codecs are appended to the list.
*/
private final CopyOnWriteArrayList<TypeCodec<?>> codecs;| TypeCodec<?> codec = cache.get(cacheKey); | ||
| TypeCodec<?> codec = PRIMITIVE_CODECS.get(cqlType); | ||
| if (codec != null && (javaType == null || codec.accepts(javaType))) { | ||
| logger.trace("Returning cached codec {}", codec); |
There was a problem hiding this comment.
Trivial: we could log "Returning primitive codec {}" here, to distinguish from the other case.
|
Closing in favor of #806 . |