2016-08-23 16:51 GMT+02:00 Clebert Suconic <[email protected]>:
> TBH: I don't expect the implementation to ever change on a given > classLoader. > > So you accept to not work for tomcat, tomee, geronimo, wildfly (ok they will not use that jar), karaf etc...? Point is a serious amount of users rely on redeployment so we have to support cleaning up somehow. > Different classLoader could have different implementations (think of a > WAR deployed on the application server). > > This is why I don't want a single SoftRef, the API being in the server by spec and the impl potentially being in the war would be broken for that exact reason. > We could offer a method to cleanup the cache and someone could use it > for a possible exceptional case. (maybe for testcases). > > I'm fine with that even on a server perspective. We can even start with http://svn.apache.org/repos/asf/geronimo/specs/trunk/geronimo-jcache_1.0_spec/src/main/java/javax/cache/Caching.java registry like and enhance it on demand > 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 >
