geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Clebert Suconic <clebert.suco...@gmail.com>
Subject Re: PR Merge?
Date Sun, 21 Aug 2016 00:03:36 GMT
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 <rmannibucau@gmail.com>
wrote:

> Le 19 août 2016 18:54, "Clebert Suconic" <clebert.suconic@gmail.com
> <javascript:_e(%7B%7D,'cvml','clebert.suconic@gmail.com');>> 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 <rmannibucau@gmail.com
> <javascript:_e(%7B%7D,'cvml','rmannibucau@gmail.com');>> wrote:
> >>
> >> Le 19 août 2016 18:06, "Clebert Suconic" <clebert.suconic@gmail.com
> <javascript:_e(%7B%7D,'cvml','clebert.suconic@gmail.com');>> 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 <rmannibucau@gmail.com
> <javascript:_e(%7B%7D,'cvml','rmannibucau@gmail.com');>> wrote:
> >> >>
> >> >>
> >> >>
> >> >> 2016-08-19 16:48 GMT+02:00 Mark Struberg <struberg@yahoo.de
> <javascript:_e(%7B%7D,'cvml','struberg@yahoo.de');>>:
> >> >>>
> >> >>> 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 <
> rmannibucau@gmail.com
> <javascript:_e(%7B%7D,'cvml','rmannibucau@gmail.com');>>:
> >> >>> >
> >> >>> >
> >> >>> >
> >> >>> > 2016-08-19 15:40 GMT+02:00 Clebert Suconic <
> clebert.suconic@gmail.com
> <javascript:_e(%7B%7D,'cvml','clebert.suconic@gmail.com');>>:
> >> >>> > 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 <
> rmannibucau@gmail.com
> <javascript:_e(%7B%7D,'cvml','rmannibucau@gmail.com');>> 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" <johndament@apache.org
> <javascript:_e(%7B%7D,'cvml','johndament@apache.org');>> 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

Mime
View raw message