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 Tue, 23 Aug 2016 14:55:03 GMT
2016-08-23 16:51 GMT+02:00 Clebert Suconic <clebert.suconic@gmail.com>:

> TBH: I don't expect the implementation to ever change on a given
> classLoader.
>
>
So you accept to not work for tomcat, tomee, geronimo, wildfly (ok they
will not use that jar), karaf etc...? Point is a serious amount of users
rely on redeployment so we have to support cleaning up somehow.


> Different classLoader could have different implementations (think of a
> WAR deployed on the application server).
>
>
This is why I don't want a single SoftRef, the API being in the server by
spec and the impl potentially being in the war would be broken for that
exact reason.


> We could offer a method to cleanup the cache and someone could use it
> for a possible exceptional case. (maybe for testcases).
>
>
I'm fine with that even on a server perspective.

We can even start with
http://svn.apache.org/repos/asf/geronimo/specs/trunk/geronimo-jcache_1.0_spec/src/main/java/javax/cache/Caching.java
registry like and enhance it on demand


> 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