search

quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.49k stars 2.59k forks source link

Optimise RESTEasy for closed world #4345

Closed Sanne closed 3 years ago

Sanne commented 4 years ago

Profling a Quarkus application it's immediatley visible that the current RESTEasy extension isn't taking advantage of the closed world assumption:

Annotation scanning and type introspection:

Stack Trace TLABs   Total TLAB Size(bytes)  Pressure(%)
java.lang.reflect.Method.getParameterTypes()    28  137,118,288 6.174
   sun.reflect.annotation.AnnotationInvocationHandler.invoke(Object, Method, Object[])  28  137,118,288 6.174
      com.sun.proxy.$Proxy17.annotationType()   16  77,359,528  3.483
         org.jboss.resteasy.spi.util.FindAnnotation.findAnnotation(Annotation[], Class) 16  77,359,528  3.483
            org.jboss.resteasy.plugins.providers.sse.SseEventSinkInterceptor.filter(ContainerRequestContext)    16  77,359,528  3.483

And more..

Stack Trace TLABs   Total TLAB Size(bytes)  Pressure(%)
java.lang.Class.getInterfaces() 425 2,083,949,208   93.826
   org.jboss.resteasy.core.providerfactory.Utils.createHeaderDelegateFromInterfaces(Map, Class[])   219 1,071,276,576   48.233
   org.jboss.resteasy.core.providerfactory.Utils.createHeaderDelegate(Map, Class)   206 1,012,672,632   45.594

I suppose this implies we can improve it a lot still?

The above methods have been identified when looking for strong allocators; this implies memory consumption could be cut down by dodging these operations.

Sanne commented 4 years ago

@patriot1burke could you take this or assign someone?

emmanuelbernard commented 4 years ago

I read it as RESTEasy reads the annotation on the method being call for every request without even caching anything or preparing its metadata? Is that correct?

Sanne commented 4 years ago

I read it as RESTEasy reads the annotation on the method being call for every request without even caching anything or preparing its metadata? Is that correct?

Correct, these are profiling snapshots taken at runtime: under load, after the application booted and after it completed warmup.

Sanne commented 4 years ago

@geoand assiging this to you :)

@asoldano could you help / mentor ? We might need some fixes from resteasy too

geoand commented 4 years ago

For the JSON part of things, I'll try and revive #3553 which has fallen far behind the latest RESTEasy + JSON-B developments.

asoldano commented 4 years ago

@Sanne yes, I can help, or ask someone in the team to do that if I run out of time. As for #3553, is it really related to what this issue is about?

geoand commented 4 years ago

@asoldano #3553 is more related to #4341 and that is the one I will be looking at mostly since I am a lot more familiar with it :).

geoand commented 4 years ago

I took a super quick look at part of this and it seems like

org.jboss.resteasy.core.providerfactory.Utils.createHeaderDelegate

is being used heavily in the glue code in Quarkus

(see io.quarkus.resteasy.runtime.standalone.VertxHttpResponse#transformHeaders).

I'll take a closer look soon

geoand commented 4 years ago

A first simple PR for RESTEasy has been opened here

geoand commented 4 years ago

@Sanne I am not seeing SseEventSinkInterceptor anywhere in the benchmarks I am running. Is there any specific one you see it in?

geoand commented 4 years ago

Added another RESTEasy PR to address a small performance optimization.

This will also require a tiny modification in io.quarkus.resteasy.runtime.standalone.VertxUtil#extractHttpHeaders to take advantage of the fix.

Sanne commented 4 years ago

@Sanne I am not seeing SseEventSinkInterceptor anywhere in the benchmarks I am running. Is there any specific one you see it in?

@geoand I've seen those running the POC for techempower which I shared here:

When reporting this I was testing Quarkus / master 2 weeks ago, so I guess it's possible other things changed in the mean time -- I'll run this all again when we have those RESTEasy patches merged, I guess ignore those for now if you can't reproduce it.

geoand commented 4 years ago

Added one more RESTEasy PR here.

geoand commented 4 years ago

The last PR was closed because it could lead to a DoS but maybe we could just hide it behind a flag that could only be turned in certain circumstances?

asoldano commented 4 years ago

Actually, I think you might want to experiment with a fixed size cache (you could clear up the cache once it reaches a configurable size threshold).

geoand commented 4 years ago

@asoldano do you have any candidate classes in mind?

Sanne commented 4 years ago

@asoldano does RESTEasy have a caching implementation around? We have a cache based on Caffeine based in Quarkus; I guess we could use that one, if RESTEasy could allow to "inject" a custom cache implementation.

geoand commented 4 years ago

@Sanne the cache would mostlikely move to Quarkus anyway - at least based on the latest implementation I had done that only cached things when a request was successful.

geoand commented 4 years ago

@Sanne the problem would be that we would have to include the caffeine extension in the quarkus-resteasy extension. Do we really want to do that?

asoldano commented 4 years ago

@Sanne @geoand , RESTEasy has a caching mechanism, but that's really for the HTTP Cache control only. I also unsure the scenario here deserves relying on Caffeine, creating a mechanism for plugging that into RESTEasy, etc. How about a ConcurrentHashMap that's evicted when it reaches a given size? The threshold could be controlled with a property (MP Config), the cache migh even be disabled by default (say, default threshold is 0)

geoand commented 4 years ago

@asoldano I'm fine with that. Let me create a RESTEasy and a corresponding Quarkus PR with that I have in mind when I say that I want to move the caching stuff into Quarkus.

Sanne commented 4 years ago

How about a ConcurrentHashMap that's evicted when it reaches a given size?

well that's better than nothing but it's not ideal. If someone had a large anough application to cross the threshold with legit URLs you'd occasionally see performance go up/down. And in case of non-legit URLs it's again exposing you to DDOs by both adding load on the dumb cache and by slowing down the system by wiping legit cache users - compounding the load which is not good when under attack.

A true cache would evict the less "valuable" entry.

I also unsure the scenario here deserves relying on Caffeine, creating a mechanism for plugging that into RESTEasy,

Right I don't mean you to have RESTEasy commit on Caffeine, but if you could expose an interface for us to implement, we'd be able to inject a better cache implementation (incidentally using Caffeine s we have it in Quarkus already - and because it's excellent).

problem would be that we would have to include the caffeine extension in the quarkus-resteasy extension. Do we really want to do that?

It could be optional / automatic. Other extension processors do something similar: if the other feature X is available, then enable integration code with it. Otherwise, don't.

This would imply that people adding the Caffeine extension to a RESTEasy-using app would see improved performance, without doing anything at all other than adding the extension.

asoldano commented 4 years ago

How about a ConcurrentHashMap that's evicted when it reaches a given size?

well that's better than nothing but it's not ideal. If someone had a large anough application to cross the threshold with legit URLs you'd occasionally see performance go up/down. And in case of non-legit URLs it's again exposing you to DDOs by both adding load on the dumb cache and by slowing down the system by wiping legit cache users - compounding the load which is not good when under attack.

Sure, it would be a compromise.

A true cache would evict the less "valuable" entry.

I also unsure the scenario here deserves relying on Caffeine, creating a mechanism for plugging that into RESTEasy,

Right I don't mean you to have RESTEasy commit on Caffeine, but if you could expose an interface for us to implement, we'd be able to inject a better cache implementation (incidentally using Caffeine s we have it in Quarkus already - and because it's excellent).

Well, if you're willing to go this way, that's ok with me. How about you propose a PR for this integration and then we decide whether it makes sense to have a generic enough cache interface for resteasy to be used in multiple points?

geoand commented 4 years ago

Well for the time being I think we'll be fine with keeping the caching in Quarkus like https://github.com/quarkusio/quarkus/pull/4563 suggests

Sanne commented 4 years ago

Agreed. But let's not forget to implement the proper solution later, could you track it as a subsequent improvement issue?

On Tue, 15 Oct 2019, 16:47 Georgios Andrianakis, notifications@github.com wrote:

Well for the time being I think we'll be fine with keeping the caching in Quarkus like #4563 https://github.com/quarkusio/quarkus/pull/4563 suggests

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/quarkusio/quarkus/issues/4345?email_source=notifications&email_token=AAAKMTJW5ZGZULKS6KRLB73QOXQ2DA5CNFSM4I5BQYD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBJIMWA#issuecomment-542279256, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKMTPVCXQ25TKA2FOVOQ3QOXQ2DANCNFSM4I5BQYDQ .

geoand commented 4 years ago

@Sanne Definitely! If #4563 gets in I'll open a new issue to track improvements

stale[bot] commented 4 years ago

This issue/pullrequest has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

geoand commented 3 years ago

Closing this as it has been implemented in the form of RESTEasy Reactive

Sanne commented 3 years ago

is RESTEasy Reactive the only way forward? I still see performance issues when using the "classic" one.

geoand commented 3 years ago

RESTEasy Classic certainly is not going anywhere, but our focus from a Quarkus PoV is on RESTEasy Reactive

Sanne commented 3 years ago

Ok but why close this then? I still hope this can be improved

geoand commented 3 years ago

Who is going to improve it? And where is the improvement going to go? If it's in Quarkus (which I doubt anyone will do at this point), then it would make sense to leave the ticket open. But if the fix is going to be in RESTEasy, then it's probably best to track it in the appropriate issue tracker

asoldano commented 3 years ago

Adding @jamezp here. James, I think it might be worth isolating the possible improvements that could go into RESTEasy and create JIRAs for them.