FasterXML / java-classmate

Library for introspecting generic type information of types, member/static methods, fields. Especially useful for POJO/Bean introspection.
http://fasterxml.com
Apache License 2.0
258 stars 42 forks source link

Contention in com.fasterxml.classmate.util.ResolvedTypeCache.find #40

Closed eklinger closed 6 years ago

eklinger commented 6 years ago

Hello, We are profiling our app under Yourkit and see high lock contention in com.fasterxml.classmate.util.ResolvedTypeCache.find() under load. Is this a known issue? Are there any work arounds?

Thanks! screen shot 2018-03-26 at 1 50 53 pm

cowtowncoder commented 6 years ago

No. In general type resolution should be avoided on critical path (as it can be heavy-weight). So I do not recall anyone reporting this as bottleneck. But I can see how this could become problematic for some heavy usage.

If you have improvement ideas for caching I am always open to ideas, PRs and so on. One thing that would probably be useful would be to allow plugging of custom cache implementations that would allow users to create and use ones more optimal to their use case, concurrency and so on. That could be the first step, possibly followed by included set of alternatives (and/or improving default cache used, based on testing).

eklinger commented 6 years ago

We are only using jackson to serialize our response bodies, which I guess is a critical path. But isn't this the same issue as reported here: https://stackoverflow.com/questions/49065290/why-is-the-simple-least-recently-used-cache-mechanism-used Guava has an LRU cache, could that be used here?

eklinger commented 6 years ago

On further inspection, it seems to be caused by the @Validated annotation on our controller so that we can use parameter level validation constraints (@DecimalMax, etc). We could move these into code if need be to get around this bottleneck.

cowtowncoder commented 6 years ago

What I am not quite clear on is the code path here: Jackson does not use Classmate for anything. So it is something else.

As to Guava: due to sheer huge size of Guava I wouldn't want to add dependency from Classmate. But if cache impl was pluggable, it could definitely be used by caller providing implementation.

With a quick look at cache itself I think use of even just ConcurrentHashMap could work here: lock contention would come from large number of concurrent threads locking, and cache is, currently, synchronized. It need not be, although removing locking would require losing of LRU aspect. In practice that may work quite well; just clearing the whole cache if it fills tends to work quite well.

So... I can probably have a look at both modularizing code (to allow pluggable impl), and perhaps changing of default implementation.

You may still want to try to figure out what is using Classmate for type resolution and see if there are external caching opportunities; something that could be used to avoid calls.

eklinger commented 6 years ago

Correct - it looks like it's the hibernate validator triggering it.

cowtowncoder commented 6 years ago

One other thing: it would make no sense for framework to read annotations for every read/write call -- annotations are static data on classes and can not change in-between. So it would seem potentially wasteful if such introspection is triggered for each request. In a way contestion would be a symptom of something else going wrong.

This is not to say that caching changes would not be valuable (I will try to investigate that). Just that sometimes there's bigger problem masked by smaller one.

cowtowncoder commented 6 years ago

@eklinger I implemented #41 which allows use of alternative ConcurrentTypeCache (pass to TypeResolver constructor), as well as custom implementations using, say, Guava's caches. I would be interested in results if you have time to try out 1.4.0 which I just released with the change.

eklinger commented 6 years ago

Hi @cowtowncoder , that's great to hear you have provided a solution. I think @gsmet from Hibernate validator team will need to take a look at this and incorporate into a new Hibernate release. Thanks!

gsmet commented 6 years ago

Hi @eklinger ,

Thanks for mentioning me. I would prefer to understand first what's exactly going on with this contention. I did a lot of profiling on HV 6 and it never occurred to me.

We shouldn't use Classmate at runtime so I'm wondering if you're not creating a new ValidatorFactory for every call/thread? This is not what you should do. The ValidatorFactory should be instantiated only once in the lifetime of your application: it gathers and cache the metadata once and for all for every bean type validated. Even the Validator can be created only once and shared (but it's less critical).

If it's not the issue (but I'm pretty sure it is), then I would need some self contained test case to reproduce it. You can open an issue on https://hibernate.atlassian.net/secure/Dashboard.jspa with it.

Thanks!

cowtowncoder commented 6 years ago

@gsmet That makes a lot of sense to me as resolve calls typically should not be on critical path for reasons you mention. I have seen similar issues with cache contention being sign of something else being used in sub-optimal way, and ending up sort of "collateral contention".

eklinger commented 6 years ago

Hello, It looks like we were still using hibernate validator 5.4.1. I will upgrade to 6 and see if the issue still occurs. We are using validator indirectly from Spring's validation framework in our Rest controllers. Thanks! Evan

gsmet commented 6 years ago

@eklinger we made a lot of performance improvements in 6.0.x but I don't think it will change the behavior you see here.

Could you share a simple setup allowing me to reproduce what you see?

gsmet commented 6 years ago

@cowtowncoder yeah it's a common misuse of our stack, unfortunately.

cowtowncoder commented 6 years ago

Implemented via #41