geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Romain Manni-Bucau <rmannibu...@gmail.com>
Subject Re: PR Merge?
Date Fri, 19 Aug 2016 14:59:32 GMT
2016-08-19 16:48 GMT+02:00 Mark Struberg <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
> >:
> >
> >
> >
> > 2016-08-19 15:40 GMT+02:00 Clebert Suconic <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>
> 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> 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
> >
> >
>
>

Mime
View raw message