Performance bottleneck in TypeFactory._fromClass

[sdonovanuk]

When using 16 cores to process json, we are seeing a performance bottleneck in TypeFactory._fromClass(..). The function contains a call to synchronized(..). Could you please consider an alternative locking strategy? Simplest fix might be a reader/writer lock.

Many thanks!

Sean

[cowtowncoder]

Could you add bit more information on location of the bottleneck? Sounds like it should be related to caching (meant to avoid unneeded re-resolution). And multi-core use cases are important to cater for, so I'd be happy to resolve the issue you are facing. But bit more info would help.

[sdonovanuk]

My test example case has 16 cores, each calling the following during their processing:

Object.readValue(JsonParser, Class<T>)  
    or
Object.readValue(String, Class<T>)

These trigger TypeFactory._fromClass(..) to resolve the type. A YourKit profile is telling me that the function is getting blocked on the synchronized(..) within the method.

If it helps, have attached a partial YourKit screenshot of one of the threads running:

Is there anything else I can get?

Many thanks!

Sean

[cowtowncoder]

I'll have a look.

Beyond avoiding locking, it is quite possibly there are ways to avoid resolution itself, i.e. this could also be a side-effect of some other performance issue (not reusing some objects that should or such). But I'll let you know what I find.

[cowtowncoder]

Ok. With version 2.3, the only places I see are short sync blocks around LRUMap, which is a LinkedHashMap sub-class. And while one could use concurrent variants, this just doesn't seem like it should not be called extensively enough to matter.
Which version are you using?

There are couple of work-arounds that could help. Basically the goal of TypeFactory is to construct JavaType instances out of Class (and TypeReference); use of JavaType has no sync blocks. Two ways to do this are:

1. Use ObjectReader instead of ObjectMapper, holding on to ObjectReader:
   final ObjectReader typeXReader = mapper.reader(TypeX.class);
   TypeX value = typeXTreader.readValue(src);
2. Construct JavaType, pass that instead of Class:
   final JavaType typeXType = mapper.constructType(MyType.class);
   TypeX value = mapper.readValue(src, typeXType);

these are actually preferable anyway (ObjectReader can also eliminate overhead of looking up JsonDeserializer for root type), although performance benefits may not be enough to convert code just to get speed up (on a good day you might see 5-10% speed up for small classes etc, nice but nothing major).

[sdonovanuk]

Thanks for looking into it. The problem is that the code that calls readValue(..) doesn't use concrete types -- it is also generic. It is part of our application's serialization framework. To implement either of the workarounds it means creating an additional cache. That seems unnecessary, given that the map already exists within TypeFactory. In our use-case, the lookup on LRUMap does occur each time -- use of a concurrent variant could be worth it.

Sean

[cowtowncoder]

Ok. I suspect that use of a read/write lock could help here, presumably most accesses are reads. Challenge for me would be verification of improvement; I would not want to make a change without having a way to confirm that it helps with reported problem.
The challenge with using ConcurrentHashMap is that a maximum size limit is also needed and combining this (which is easy and safe with LinkedHashMap).
But use of simple ReentrantReadWriteLock sounds doable, if there was a way to verify resulting performance improvements.

[sdonovanuk]

We would happily compile up and test any code changes in our environment and forward the profiled results. I would need 1-2 days to do it though.

Sean

[cowtowncoder]

Sounds like deal -- I hope to find time tonight. If it could be tested on 2.4.0-SNAPSHOTs (for jackson-core), that'd be easiest, I could push snapshots. Or if not, could speculatively check in 2.3 branch, given that it should be small change all around.

[sdonovanuk]

Please don't work of an evening on my account! It's not that important. We're currently using 2.3. Using 2.4 could be problematic, as we haven't tried yet -- seems like some potential to encounter some API differences. Maybe not.

[cowtowncoder]

:)

That's ok -- I tend to work on Jackson during off-hours. So not extra work, just depends on when I happen to have time and work on related area.

On 2.3 vs 2.4: I do not expect you would be running with 2.4, and if this worked well, I would backport it for 2.3.3. Official 2.4 is not yet out, and may take a while anyway.
I would just prefer working on mainline that's all; but like I said it is not a big deal either way.

I'll update this issue when I have the patch, either way.

[cowtowncoder]

Ok: I checked in a change that now uses read/write lock for LRU cache, and should perform better.
While it should be safe change I would like a multi-threaded test for it both from correctness (no transient failures) and performance perspective. I will push 2.4.0-SNAPSHOT to maven snapshots, but the best way may be local build, if possible.

[sdonovanuk]

Acknowledged. Will test and post-results in next few days. Many thanks!

[cowtowncoder]

Perfect. Looking forward to the test results.

[cowtowncoder]

I assume this works, closing.

[cowtowncoder]

Ouch. As per #503, looks like change can not be used, and I will need to consider alternative take.