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] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > >> > >> Le 19 août 2016 18:06, "Clebert Suconic" <[email protected] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > >> >> > >> >> > >> >> > >> >> 2016-08-19 16:48 GMT+02:00 Mark Struberg <[email protected] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[email protected]');>>: > >> >>> > > >> >>> > > >> >>> > > >> >>> > 2016-08-19 15:40 GMT+02:00 Clebert Suconic < > [email protected] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[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] > <javascript:_e(%7B%7D,'cvml','[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
