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 Sun, 21 Aug 2016 08:24:27 GMT
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" <clebert.suconic@gmail.com> 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 <rmannibucau@gmail.com>
> wrote:
>
>> Le 19 août 2016 18:54, "Clebert Suconic" <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>
>> wrote:
>> >>
>> >> Le 19 août 2016 18:06, "Clebert Suconic" <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> wrote:
>> >> >>
>> >> >>
>> >> >>
>> >> >> 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
>> >> >>> >
>> >> >>> >
>> >> >>>
>> >> >>
>> >> >
>> >> >
>> >> > --
>> >> > Clebert Suconic
>> >> >
>> >
>> >
>> >
>> > --
>> > Clebert Suconic
>> >
>>
>
>
> --
> Clebert Suconic
>
>

Mime
View raw message