use the sharedLockFreePool as default pool instead of allocating a new one

[mariofusco]

At the moment jackson allocates a new pool every time the default pool is asked. I believe it would be more correct to always reuse the shared one also because the (useless?) creation of a new one is not only causing this issue in quarkus, but even worse is very likely the main cause of the performance problem reported here.

[cowtowncoder]

No, I don't think global singleton is a good choice; it tends to be risky conceptually.

Users can configure to do that, but should be (IMO) opt-in and not default.

[pjfanning]

@cowtowncoder users are just going to create one JsonFactory and use that to create all their mappers. That would have the same effect of sharing the recycler pool across many threads. Why not make it the default? It will take everyone time to rewrite their code. People switching up to Jackson 2.17 are not expecting to have to rewrite their code - many of them don't even know what a Jackson JsonFactory is. They just create JsonMapper/ObjectMappers directly.

[pjfanning]

@mariofusco can you change the Quarkus code to create a singleton JsonFactory and to use that to create all Jackson mappers? I would still like to see this merged but Quarkus can be changed while we decide on what to do with Jackson.

[cowtowncoder]

@mariofusco can you change the Quarkus code to create a singleton JsonFactory and to use that to create all Jackson mappers? I would still like to see this merged but Quarkus can be changed while we decide on what to do with Jackson.

No... Do not use JsonFactory for creating mappers; ensure singleton (or close) mappers are being used. That's how factories are reused normally.

But that is already what Quarkus does isn't it. No one is creating per-call JsonFactory or ObjectMapper instances, unless they want to get abysmally bad performance.

[cowtowncoder]

I don't get this assumption that everyone is creating throw-away JsonFactory instances. I don't think that is common at all; and assuming that may hide actual other problems.

[cowtowncoder]

One further thought: the access really should not be called by framework code: if default choice is desired, that's what JsonFactory will use anyway and get instance it needs without external help. And in fact explicit call is somewhat wasteful.

So why is Quarkus calling it?

[cowtowncoder]

People switching up to Jackson 2.17 are not expecting to have to rewrite their code - many of them don't even know what a Jackson JsonFactory is. They just create JsonMapper/ObjectMappers directly.

Yes. And that should work just fine -- if you create ObjectMapper and reuse THERE SHOULD BE NO PERFORMANCE PROBLEM. They need not know anything about JsonFactory; there is no expectation they would need to.
That's my whole point: if there is a problem, something else is wrong.
And I don't think making speculative changes to work around not-understood-problem is the way to go.

[pjfanning]

People switching up to Jackson 2.17 are not expecting to have to rewrite their code - many of them don't even know what a Jackson JsonFactory is. They just create JsonMapper/ObjectMappers directly.

Yes. And that should work just fine -- if you create ObjectMapper and reuse THERE SHOULD BE NO PERFORMANCE PROBLEM. They need not know anything about JsonFactory; there is no expectation they would need to. That's my whole point: if there is a problem, something else is wrong. And I don't think making speculative changes to work around not-understood-problem is the way to go.

The point is lots of users create mappers over and over. This was already slower than reusing mappers but now we've made this much slower than reusing mappers.

[cowtowncoder]

@pjfanning I have little sympathy for cases where ObjectMappers are not reused to some degree.

[pjfanning]

@mariofusco based on the discussion so far, it seems best if you look at Quarkus and see if you can reuse ObjectMappers instead of creating lots of them. There is more to be gained by reusing mappers than just reusing the Buffer Recycler. There are Deserialization and Serialization caches among other things stored at the ObjectMapper level and you don't benefit from these if you throw away the mapper instances.

[cowtowncoder]

I filed #1256 for actually going back to threadLocalPool(), as sort of safest thing to do, to give us more time to figure out what to do with default setting.

[cowtowncoder]

Realized I hadn't actually replied to this:

@cowtowncoder users are just going to create one JsonFactory and use that to create all their mappers. That would have the same effect of sharing the recycler pool across many threads. Why not make it the default?

My concern is not with users fully controlling things, but rather different components sharing global pool. Jackson tries very hard to be compartmentalized so that use by one component/library/framework should be well isolated from that of others.
Because of this I am very cautious wrt instances (mapper, JsonFactory) sharing anything at all ("share nothing") -- including but not limited to global system properties for configuration. And now recycler pools.
(ThreadLocal-based one is sort of against this, but it is strictly bound by number of threads so pool cannot extend indefinitely)

I agree that for single user it makes sense to effectively share a single pool; I am just worried about cases where different usage collides.

I do realize that it is very difficult to really reason about all or even major usage across existing user base, and so discussion tends to be quite theoretical.

Finally: one thing I may also be open to, wrt 2.18, is static global setting to override the global default. Maybe it'd be possible to specify lambda to use for overriding default pool implementation returned -- and as usual, with strong warnings that being global singleton setting, it is not to be changed by frameworks and libraries, but only by whoever deploys full applications.