-
Notifications
You must be signed in to change notification settings - Fork 98
Performance optimizations #92
Comments
Hi sounds good. Here is some feedback:
What would really help if you can do a separate pull request for each topic because the changes sound big. |
Hi again,
I'm not sure I can separate everything easily but I'll discuss it and get back to you :) |
Alright so 4) and 5) are going to drop out of this PR. As for 4) I can already use Jersey's ApplicationEventListener as an OSGi Service with @Provider annotation and that works great :) |
I have one question, if a resource is removed the application is not marked as dirty -> Line 46 in c03759e
why is that? Shouldn't jersey context be reloaded when we are removing resources? |
Definitely. Seems you found a bug. |
Ok I will include the fix in this PR as well :) |
Alright, can you also add a testcase for this? It seems it was missing. |
Yes, I will, for this PR I'll have to remove, add and rework a lot of tests anyway. |
Do you happen to have an eclipse formatter for the publisher bundle? I can see that there is a custom one defined in the project settings but for some reasons it messes up your license headers + javadocs and removes any emtpy lines within the code? Is this intended? |
No, it's not intended to mess it up :) Just use the format you are used to. I will go over the change anyway ;). I'm kind of a paranoid developer... |
I've just created the PR. Unfortunately I was not able to separate the issues easily. Hopefully the changes are not too much. |
I've identified several things I intend to fix in a pull request soon that affect performance especially on slower embedded devices. Firstly I'm going to say that Jersey initialization/reload is really slow on the particular embedded device I'm testing with, so in light of that there are several problems:
ResourceTracker - in the addingService method if a service does not have jax-rs annotations we should unget the service and return null in order to not track the service anymore. This also allows us to remove the isResource() checks in the {removed,modified}Service methods.
Currently the connector is tracking ApplicationConfiguration, ServletConfiguration and also ConfigAdmin Configuration, when these three appear the way things are handled a new JerseyContext is created - so on a slower device we may end up with up to 4 Jersey initializations as follows:
2.1) Once when first resource appears
2.2) Another one when ConfigAdmin Configuration appears
2.3) Another one when ApplicationConfiguration appears
2.4) And a last one when ServletConfiguration appears
On a device where Jersey configuration is really slow (10-20 seconds) this is quite a heavy hit.
I intend to optimize this by including Configuration/ApplicationConfiguration/ServletConfiguration into the same/similar mechanism that is used by Resources (using publishDelay).
This is related to 2.1). When the first resource appears we register our servlet with OSGi - which is fine but this currently causes Jersey ServletContainer.init() to be called which initializes jersey, then after resources all appear we reload jersey - so at minimum two Jersey loads in most cases. Another problem is that while Jersey is reloading asynchronously we could try to access our servlet and we get a nasty exception from Jersey while that is happening. I intend to have the ServletContainerBridge extend HttpServlet and wrap the Jersey ServletContainer, so if Jersey is loading/reloading we can return Error 503 not available for example.
In relation to 3) I think it's good to have some sort of listener/hook mechanism before and after the Jersey Asynchronous initialization is performed. I currently need to include some device specific code to disable security checks for the Jersey thread and would also be good to know when Jersey is fully ready.
Possibly also include a way to delay initialization until it is manually triggered. This way I can trigger Jersey initialization at a time where all my resources should already be registered and avoid most of the problems described in 2). Of course if no such trigger is present we would fall back to the default behavior.
Would really appreciate if you let me know whether or not you feel this is something you would consider merging as a heads up :)
Thanks in advance,
Ivan
The text was updated successfully, but these errors were encountered: