TBH: I don't expect the implementation to ever change on a given classLoader.
Different classLoader could have different implementations (think of a WAR deployed on the application server). We could offer a method to cleanup the cache and someone could use it for a possible exceptional case. (maybe for testcases). On Sun, Aug 21, 2016 at 6:43 AM, Romain Manni-Bucau <[email protected]> wrote: > Le 21 août 2016 12:24, "Mark Struberg" <[email protected]> a écrit : >> >> We should imo check the spec wording. >> If it's not specified in >> >> * the spec PDF >> * the official JavaDoc >> * the TCK >> > > There is nothing > >> then we are pretty free to implement it however we think it's best. >> >> >> Most of the 'standard' spec api jars e.g. don't support OSGi, but in >> Geronimo we do. >> > > Keep it mind it would also affect EE and tomee for instance, not only osgi > which should rely on providerlocator - not yet done afaik. > >> >> LieGrue, >> strub >> >> >> >> >> On Sunday, 21 August 2016, 10:24, Romain Manni-Bucau >> <[email protected]> wrote: >> >> >> > >> > >> >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/gero nimo-specs/pull/4 >> >>>>> >>> > >> >>>>> >>> > John >> >>>>> >>> > >> >>>>> >>> > >> >>>>> >>> > -- >> >>>>> >>> > Clebert Suconic >> >>>>> >>> > >> >>>>> >>> > >> >>>>> >>> >> >>>>> >> >> >>>>> > >> >>>>> > >> >>>>> > -- >> >>>>> > Clebert Suconic >> >>>>> > >> >>>> >> >>>> >> >>>> >> >>>> -- >> >>>> Clebert Suconic >> >>>> >> >> >> >>-- >> >>Clebert Suconic >> >> >> >> >> > >> > -- Clebert Suconic
