Jackson LockFreePool recycler pool causes memory issues

[davseitsev]

After upgrade Trino from 409 to 443 we started to have memory issues. Some workers disappear from the cluster due to intensive major GC. In the logs we can see messages like:

INFO    Notification Thread     io.airlift.stats.JmxGcMonitor   Major GC: application 61ms, stopped 9353ms: 91.59GB -> 91.13GB

On the graphs we can see memory starvation:

Heap dump analisys shows it comes from JsonUtil#OBJECT_MAPPED_UNORDERED. It creates JsonFactory with default implementation of RecyclerPool which is LockFreePool. This implementation is relatively young, has some concerns and is considered not to be used as default FasterXML/jackson-core#1256

We have change the factory below to use JsonRecyclerPools.threadLocalPool()

trino/core/trino-main/src/main/java/io/trino/util/JsonUtil.java

Lines 118 to 121 in f0d5b0f

	public static JsonFactory createJsonFactory()
	{
	return jsonFactoryBuilder().disable(CANONICALIZE_FIELD_NAMES).build();
	}

I'm not sure it's the best implementation for Trino but now we can't reproduce the issue.

[hashhar]

should be now addressed with #21361.

Can you validate the change @davseitsev if possible since you seem to have a way to reproduce this?

[davseitsev]

@hashhar I've applied your fix and testing it right now. I will update later.

[davseitsev]

Looks good, I can't reproduce the issue. I will keep testing and update if anything changes.

[wendigo]

Closing then. Release should happen this week. Thanks!

[marvin-roesch]

This issue appears to still be affecting us on Trino 444. We are running into massive memory issues with this. In our case, heap dumps indicate that JsonTypeUtil.parseJson is at fault.

Some quick debugging shows that the issue is due to the SORTED_MAPPER.writeValue call using the default JsonGenerator instance which in turn gets created by a default JsonFactory that uses the LockFreePool implementation. Until the upstream maybe changes back the default again, it might be needed to perform a more systematic survey of Jackson usages to make sure this doesn't negatively affect more places.

Ideally, all ObjectMapper instances would get created with the customized JsonFactory that contains the configuration.

Alternatively, maybe explicitly downgrading the Jackson libraries is an option?

[wendigo]

SORTED_MAPPER is using ObjectMapperProvider which configures the JsonFactoryBuilder to use JsonRecyclerPools.threadLocalPool(). if this is ineffective I guess we need to report Jackson issue.

Update: it was fixed in master so not yet been released (part of Airlift 244 update: airlift/airlift@6d36ba3 and 00d867b)

Please test with 445 once it's released.

[marvin-roesch]

That's great to hear, thanks! I wasn't checking other dependencies' upstream when investigating this. Will report back once 445 is out.

[mariofusco]

I'm one of the author of the new Jackson pool and I would like to investigate the memory issue it caused, but at the moment I haven't been able to reproduce it on my side. Can anybody please explain how I could reproduce this problem? Thanks.

[mariofusco]

Note that we're discussing this issue also here, so in case you have any suggestion on how to reproduce the problem it would be great to report it also there. In particular it would be very interesting if you could give a look at this comment and let me know if this could be relevant for your environment.

[wendigo]

@mariofusco have you seen how we are parsing JSON: https://github.com/trinodb/trino/blob/master/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/util/JsonTypeUtil.java#L46 ?

[cowtowncoder]

I am also hoping to work on figuring out the underlying issue. I created a test:

FasterXML/jackson-core#1265

which suggests some abnormal temporary growth in retention, but not quite the smoking gun. So hoping to learn if there is something specific that might trigger or amplify issues.

Another thing that would be good to know -- although I realize it may not be easy to try it out -- is whether use of one of alternate new pools, JsonRecyclerPools.newConcurrentDequePool() -- would show good performance without requiring use of ThreadLocal. The reason I am interested in this choice is that for Jackson 2.17.1 there is need to change the default and choices are reverting back to pre-2.17 ThreadLocal-pool or concurrentDequePool. And it would be great to base decision on observed performance from a real concurrency-heavy use case which I think Trino would qualify as.

[marvin-roesch]

@wendigo This situation has improved for us now that we're on 445 with the explicit ThreadLocalpool setting. Unfortunately, however, there still appear to places where an ObjectMapper is created without any additional settings, which still surfaces this problem, albeit at a much slower pace.

A heap dump I took indicates JsonInputFunctions is the majority contributor this time around.

Jackson 2.17.1 has been released with a revert to the old pool as default, are there plans to incorporate that into airlift and an upcoming Trino release?

[wendigo]

@marvin-roesch jackson is already updated in master, will be released with 447

[wendigo]

Follow-up to address Json[Input|Output]Functions ObjectMapper usage: #21867