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 Tue, 23 Aug 2016 14:51:06 GMT
TBH: I don't expect the implementation to ever change on a given classLoader.

Different classLoader could have different implementations (think of a
WAR deployed on the application server).

We could offer a method to cleanup the cache and someone could use it
for a possible exceptional case. (maybe for testcases).

On Sun, Aug 21, 2016 at 6:43 AM, Romain Manni-Bucau
<rmannibucau@gmail.com> wrote:
> Le 21 août 2016 12:24, "Mark Struberg" <struberg@yahoo.de> 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
>> <rmannibucau@gmail.com> 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" <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/gero nimo-specs/pull/4
>> >>>>> >>> >
>> >>>>> >>> > John
>> >>>>> >>> >
>> >>>>> >>> >
>> >>>>> >>> > --
>> >>>>> >>> > Clebert Suconic
>> >>>>> >>> >
>> >>>>> >>> >
>> >>>>> >>>
>> >>>>> >>
>> >>>>> >
>> >>>>> >
>> >>>>> > --
>> >>>>> > Clebert Suconic
>> >>>>> >
>> >>>>
>> >>>>
>> >>>>
>> >>>> --
>> >>>> Clebert Suconic
>> >>>>
>> >>
>> >>--
>> >>Clebert Suconic
>> >>
>> >>
>> >
>> >



-- 
Clebert Suconic

Mime
View raw message