-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Rework synchronized block from BeanDeserializerBase
#4458
Rework synchronized block from BeanDeserializerBase
#4458
Conversation
@cowtowncoder does this change look ok? ConcurrentHashMap better hanndled concurrent calls and the lock is only used to ensure that the map is lazily created once and once only. |
/** | ||
* Lock used to prevent multiple creation of _subDeserializers map | ||
*/ | ||
private final ReentrantLock _subDeserializersLock = new ReentrantLock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now adds ReentrantLock for every BeanDeserializer
instance, regardless of whether polymorphic handling is enabled. PM handling likely being a small minority of all usage.
src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializerBase.java
Show resolved
Hide resolved
Not sure; fundamentally this adds a lock for vast majority of deserializers that are for non-polymorphic handling. This seems like a case where it'd be good to figure it out if there is an actual performance problem to solve, instead of proactively making changes. |
@cowtowncoder I have taken out the ReentrantLock and use a synchronized block again - but the refactor in this PR means it won't be hit as often. Could you consider the refactored PR? |
BeanDeserializerBase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this makes sense.
Yes, approved, will merge. |
Changed to use a ConcurrentHashMap internally so we don't have to lock at all - with one remaining exception.
The map is transient so we need to ensure that when we create it - that only one thread can do this. We have retained the synchronized block just for this part.