Just need to copy it from other spec. I have nothing against it. Alternative is in johnzon to return an impl instead of using the provider again. Maybe sthg to investigate
Le 21 août 2016 02:03, "Clebert Suconic" <[email protected]> a écrit : > I Will be back from vacations on Tuesday. I could do some of this > weakhashmap per classloader but I don't want to waste time if you wouldn't > merger it anyways. Let me know what you think. > > On Friday, August 19, 2016, Romain Manni-Bucau <[email protected]> > wrote: > >> Le 19 août 2016 18:54, "Clebert Suconic" <[email protected]> a >> écrit : >> > >> > Is it really a requirement to change the provider within the same class >> loader? >> > >> >> * not the same but same jvm >> >> > >> > My expectation is to always return the same. When the class loader is >> gone the factory itself will be gone. >> > >> >> Nop in classloader graph or tree (osgi / ee) >> >> > >> > Or you could cache per class loader. Some sort of >> weakhashmap<classloader, provider>. >> > >> > >> >> Works but perf can be as bad >> >> > >> > >> > Notice that there is a separate issue on only using the context >> classloader. >> > >> > On Friday, August 19, 2016, Romain Manni-Bucau <[email protected]> >> wrote: >> >> >> >> Le 19 août 2016 18:06, "Clebert Suconic" <[email protected]> >> a écrit : >> >> > >> >> > I would rather fix the framework in one place than force everybody >> to not use the factory. (The code Change you mentioned) >> >> > >> >> > >> >> > The way you are driving this you are making the factory useless. >> >> > >> >> >> >> I didnt design the API but it is exactly the case and I would even go >> further, it is a dangerous API cause it doesnt expose a lifecycle for spec >> entities which can break apps in higher spec. Back to our current issue, it >> only works in plain standalone apps, not in tomcat (embedded or not), worse >> in OSGi, and most containers having a configurable classloader. >> >> >> >> If you want to speak of a "cache" fix you need to do it like JCache at >> least: ie by classloader which is not a "good" one but would work. Would do >> another hash which doesnt guarantee a speed improvement but wouldnt break >> the users. >> >> >> >> > >> >> > On Friday, August 19, 2016, Romain Manni-Bucau < >> [email protected]> wrote: >> >> >> >> >> >> >> >> >> >> >> >> 2016-08-19 16:48 GMT+02:00 Mark Struberg <[email protected]>: >> >> >>> >> >> >>> I’m with Clebert here. >> >> >>> >> >> >>> The current implementation in json-1.0 is a manual >> java.util.ServiceLoader (for java5 backward compat I guess). >> >> >>> >> >> >>> But it does not do a loop and tries to find the ‚closest’ one, but >> just takes the first service it finds. >> >> >>> >> >> >> >> >> >> I'd would have loved to fail, it is just aligned on the RI behavior >> which is this one and we need to keep that >> >> >> >> >> >>> >> >> >>> Consider now you have 2 impls. The first in the tomcat/lib and the >> other in your web app. >> >> >>> There is no guarantee that you always get the one from your >> webapp. It’s just random, isn’t? >> >> >>> >> >> >> >> >> >> Depends the environment, in a plain tomcat yes but in other >> containers/contexts the classloader can ensure to force this hierarchy. >> Typically in TomEE we handle it for several spec. Using this cached >> instance would break it very very badly. >> >> >> >> >> >>> >> >> >>> In that case we need to come up with a better approach anyway. OR >> we execute this only once so the container can force it’s own impl. >> >> >>> >> >> >> >> >> >> Anyway guys you seems to ignore the point that the fix is trivial >> and there is no actual performance issue once correctly coded so not sure >> it is a real issue. >> >> >> >> >> >>> >> >> >>> LieGrue, >> >> >>> strub >> >> >>> >> >> >>> PS: this seems to be a complex topic. Each spec does it different. >> And each of them has achileus heals. E.g. the CDI.setCdiProvider :( >> >> >>> >> >> >>> >> >> >>> > Am 19.08.2016 um 15:44 schrieb Romain Manni-Bucau < >> [email protected]>: >> >> >>> > >> >> >>> > >> >> >>> > >> >> >>> > 2016-08-19 15:40 GMT+02:00 Clebert Suconic < >> [email protected]>: >> >> >>> > The patch is caching it with a soft cache. Meaning the impl will >> go away when memory is needed and a class loader gone. >> >> >>> > >> >> >>> > >> >> >>> > Leaking was not in term of memory but in term of impl (the API >> should enable you to change the impl at each request if the >> classloader/app/bundle changed) >> >> >>> > >> >> >>> > >> >> >>> > The performance of having to load the class on every operation >> is really heavy. >> >> >>> > >> >> >>> > True but in an app you never do it multiple times using >> Json.xxxx but you rely on jsonXXXFactory which is looked up only once. >> (that's how johnzon mapper is implemented for instance) >> >> >>> > >> >> >>> > >> >> >>> > >> >> >>> > On Friday, August 19, 2016, Romain Manni-Bucau < >> [email protected]> wrote: >> >> >>> > Hi John >> >> >>> > >> >> >>> > I implement it this way to work in all cases and deployment >> without leaking an impl (this patch does) >> >> >>> > >> >> >>> > The fix is in artemis I suspect: Json.write/read shouldnt be >> used bit they should go through the factory. >> >> >>> > >> >> >>> > Hope it helps. >> >> >>> > >> >> >>> > Le 19 août 2016 04:11, "John D. Ament" <[email protected]> >> a écrit : >> >> >>> > Hi, >> >> >>> > >> >> >>> > I was wondering, would anyone be able to review this PR and >> perhaps merge it? After I ported Artemis to use Johnzon, Clebert noticed >> some performance issues where the service was being looked up always. His >> PR is to help resolve that. >> >> >>> > >> >> >>> > https://github.com/apache/geronimo-specs/pull/4 >> >> >>> > >> >> >>> > John >> >> >>> > >> >> >>> > >> >> >>> > -- >> >> >>> > Clebert Suconic >> >> >>> > >> >> >>> > >> >> >>> >> >> >> >> >> > >> >> > >> >> > -- >> >> > Clebert Suconic >> >> > >> > >> > >> > >> > -- >> > Clebert Suconic >> > >> > > > -- > Clebert Suconic > >
