Rework locking in DeserializerCache

[pjfanning]

I'm a little concerned that ReentrantLock is a little bit slower than synchronized - what I have done is moved some optimistic logic to before we attempt to get the lock - in an effort to possibly avoid even needing the lock in some cases where the value is cached already.

[cowtowncoder]

My concern is that incomplete deserializers handling is actually not just performance optimization, but actual correctness challenge: this because the handling of deserialization resolution to support cyclic dependencies. So creating more than one deserializer for type, during construction, can lead to linkage across incomplete deserializer instances.
Performance aspects are of less concern to me than correctness in this case.

Having said that, since this does address parts of #4430 we can proceed with this.

But I basically do not think we can necessarily start removing most of synchronization aspects due to intricate nature of cyclic-dependency resolution (would be great if we could reliably test correctness, and make locking more obvious and explicit -- but we are not there at this point).

[pjfanning]

My concern is that incomplete deserializers handling is actually not just performance optimization, but actual correctness challenge: this because the handling of deserialization resolution to support cyclic dependencies. So creating more than one deserializer for type, during construction, can lead to linkage across incomplete deserializer instances. Performance aspects are of less concern to me than correctness in this case.

I don't think the change will lead to extra creations. It just follows this common pattern.

• optimistic get - if get returns value, return it
• no cached value - lock and then check cache again just in case the previous lock owner was looking for the same thing and then created it for us
• still nothing, then create it and cache it

This only really differs from the previous code due to the optimistic get.

[pjfanning]

@oddbjornkvalsund any interest in trying this out? It should be more performant than your version because it doesn't always lock. As @cowtowncoder highlights, the code is complicated enough that this approach might be too aggressive though.

[cowtowncoder]

I think we should do #4430 for 2.17, and then possible follow-up changes like these in 2.18. This to reduce risk.

[cowtowncoder]

@pjfanning Is this ready for review?

[pjfanning]

@pjfanning Is this ready for review?

@cowtowncoder this is ready to review