Skip to content
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

Use ReentrantLocks instead of synchronized blocks #184

Merged
merged 3 commits into from
Apr 1, 2024

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Mar 28, 2024

I don't think the existing synchronized code stops multiple creations of mappers.

The new lock code double checks that the mappers are not initialized. The new locking is optimistic - we don't lock and only lock when we know we need to lazily create instances.

@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 28, 2024

I don't see much value in doing any of this proactively.

Let's wait until someone actually requests these changes.

EDIT: changed my mind, see later comments.

@pjfanning
Copy link
Member Author

I don't see why locking this code helps.

    public synchronized ObjectMapper getConfiguredMapper() {
        /* important: should NOT call mapper(); needs to return null
         * if no instance has been passed or constructed
         */
        return _mapper;
    }

How are we sure that whatever is locking is creating the _mapper? It could be just another getConfiguredMapper call.

@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 28, 2024

I don't see why locking this code helps.

    public synchronized ObjectMapper getConfiguredMapper() {
        /* important: should NOT call mapper(); needs to return null
         * if no instance has been passed or constructed
         */
        return _mapper;
    }

How are we sure that whatever is locking is creating the _mapper? It could be just another getConfiguredMapper call.

Yeah that seems dubious, not sure why it is synchronized.

----But my bigger point is that none of these methods seems likely at all to become synchronization hotspot.---
----So there seems to be no point in making changes; fixing what isn't necessarily broken (even if starting from scratch would be written differently).----

@cowtowncoder
Copy link
Member

Hmmh. Actually, I take that back -- this could have measurable performance impact. Given that getMapper() call is made for each request served.

So yeah, maybe it's worth doing after all.

@cowtowncoder cowtowncoder changed the title use ReentrantLocks instead of synchronized blocks Use ReentrantLocks instead of synchronized blocks Mar 29, 2024
@cowtowncoder cowtowncoder merged commit c40c204 into FasterXML:2.18 Apr 1, 2024
4 checks passed
@cowtowncoder cowtowncoder added this to the 2.18.0 milestone Apr 1, 2024
@cowtowncoder
Copy link
Member

@pjfanning Merged and I think this makes sense. Would it be possible to get similar PR for https://github.com/FasterXML/jackson-jakarta-rs-providers ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants