Non-cachable deserializers are still created in a single-threaded way

[winitzki]

This is related to issues #735 and #604.

Deserializers are created and cached in com.fasterxml.jackson.databind.deser.DeserializerCache#_createAndCacheValueDeserializer within a synchronized block. The comment in that file explains that "Only one thread to construct deserializers at any given point in time; limitations necessary to ensure that only completely initialized ones are visible and used". In case the deserializer is not cachable, we do not need to lock the cache with the synchronized block; we can just create a new deserializer and return it. However, the code still creates a lock and calls _createAndCache2, which then checks whether the deserializer is cachable.

So, for a deserializer that is not cachable, the lock is created even though cache is not being updated, so no locking is needed.

The result of this is a multithreading lock contention for non-cachable deserializer. In scenarios where many threads are deserializing at once, this contention leads to severe degradation of performance.

This problem has been mitigated as of version 2.8.x by enabling more deserializers to be cached. However, if serializers are not cached (because they are custom serializers with complicated behavior), performance degradation due to unnecessary locking will still occur.

Perhaps it is possible to rearrange the logic in DeserializerCache#_createAndCache2 and DeserializerCache#_createAndCacheValueDeserializer so that non-cachable deserializers are created without a global lock.

[cowtowncoder]

Interesting. Yes, I agree that avoiding synchronization for non-cachable instances would make sense.

[cowtowncoder]

Looking at the code, I am not overly optimistic about possibilities for rearranging. The problem is that cachability is only known at a rather late point (after locating/constructing said deserializer) and even without caching it is necessary to resolve recursive refefences (via _incompleteDeserializers).

[sandszhouSZ]

@cowtowncoder I faced the same Performance issues with synchronized block when running many threads；

I think we should replace synchronized block with ThreadLocal;
The perfomace doubled when I modify code locally.

[cowtowncoder]

@sandszhouSZ most of the time the real problem is not this blocking, but rather question of why are deserializers not being reused (caching is just one mechanism): synchronization is only needed when creating deserializer for the first time. I am not sure how ThreadLocal would be applicable in this case either.

[sandszhouSZ]

@cowtowncoder

I have a case like this: the inputs are map nested map

But because MapDeserializer.java will checking _valueTypeDeserializer == null, so every time the result will not be cached（though _valueTypeDeserializer may be the same）,
At the same time the creation is high cost

so many thread blocking here

i think maybe ThreadLocal will be a better choice just with Minimal memoery space overhead

final protected ThreadLocal<HashMap<JavaType, JsonDeserializer>> _incompleteDeserializers
51 = ThreadLocal.withInitial(() -> new HashMap<JavaType, JsonDeserializer>(8));
52 //final protected HashMap<JavaType, JsonDeserializer> _incompleteDeserializers
53 // = new HashMap<JavaType, JsonDeserializer>(8);

[cowtowncoder]

Depending on how your code is structured, use of ObjectReader would allow lookup of the main deserializer to be only done once and retained there, instead of attempting to find one from ObjectMapper global cache.

My concern with ThreadLocal approach really is that it seems to have a good chance of leading to unintentional memory retention (every thread will have HashMap on it -- granted, should be mostly empty), and could lead to bizarre problems with deserializer resolve() having its own local copy, yet some deserializers ending up in shared cache.
During heavy concurrent usage I would not trust this not to cause additional new failures that I would then be asked to troubleshoot.
This is why I am not particularly interested in considering this approach.