Optimise RESTEasy for closed world

[Sanne]

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]

@patriot1burke could you take this or assign someone?

[emmanuelbernard]

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]

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]

@geoand assiging this to you :)

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

[geoand]

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

[asoldano]

@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]

@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]

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]

A first simple PR for RESTEasy has been opened here

[geoand]

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

[geoand]

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]

@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:

• Quarkus fixes FroMage/FrameworkBenchmarks#1

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]

Added one more RESTEasy PR here.

[geoand]

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]

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]

@asoldano do you have any candidate classes in mind?

[Sanne]

@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]

@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]

@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]

@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]

@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]

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]

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]

Well for the time being I think we'll be fine with keeping the caching in Quarkus like #4563 suggests

[Sanne]

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, ***@***.***> wrote: Well for the time being I think we'll be fine with keeping the caching in Quarkus like #4563 <#4563> suggests — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4345?email_source=notifications&email_token=AAAKMTJW5ZGZULKS6KRLB73QOXQ2DA5CNFSM4I5BQYD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBJIMWA#issuecomment-542279256>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAKMTPVCXQ25TKA2FOVOQ3QOXQ2DANCNFSM4I5BQYDQ> .

[geoand]

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

[stale]

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]

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

[Sanne]

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

[geoand]

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

[Sanne]

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

[geoand]

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]

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