You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geronimo.apache.org by "John D. Ament" <jo...@apache.org> on 2016/08/19 02:11:39 UTC
PR Merge?
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
Re: PR Merge?
Posted by Mark Struberg <st...@yahoo.de>.
That particular concern can easily get addressed via this trick [1]:
protected static Map<ClassLoader, WeakReference<Config>> configs = Collections.synchronizedMap(new WeakHashMap<ClassLoader, WeakReference<Config>>());
LieGrue,
strub
[1] https://github.com/struberg/javaConfig/blob/master/impl/src/main/java/org/apache/geronimo/config/DefaultConfigProvider.java#L33
On Tuesday, 23 August 2016, 17:18, Romain Manni-Bucau <rm...@gmail.com> wrote:
>
>
>
>
>2016-08-23 17:15 GMT+02:00 Clebert Suconic <cl...@gmail.com>:
>
>On Tue, Aug 23, 2016 at 10:55 AM, Romain Manni-Bucau
>><rm...@gmail.com> wrote:
>>>
>>> 2016-08-23 16:51 GMT+02:00 Clebert Suconic <cl...@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.
>>
>>As Far as I know redeployment means a classLoader change. If you cache
>>per classLoader a redeployment will remove the old classLoader and add
>>a new one.
>>
>>No applicaition server that I know will change loaded classes once the
>>classLoader is up. Redeployments will remove the old classloader and
>>create a new one.
>>
>>
>
>
>"Redeployments will remove the old classloader"
>
>
>this is were we "split" I suspect. This is not true since the key is a classloader and classloader is the root of most of everything so GC is rarely enough
>
>
>>There are serious implications to the application server if that
>>doesn't work this way. So I doubt any application server would behave
>>differently.
>>
>>
>>I may be wrong, but this is what I have seen over the years.
>>
>
>Think we agree on what we want but disagree on what actually happens there.
>
>
>What about starting with the registry I proposed and enhance/fix it on demand? Sounds like a compromise
>
>
>
>
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2016-08-23 17:55 GMT+02:00 Clebert Suconic <cl...@gmail.com>:
> > Not always. Will be true until something else has a reference to the
> > classloader, like a provider Class for instance.
> > In other words it can just prevent the whole webapp to be garbage
> collected.
>
> A provider class Is not supposed to hold an explicit reference to the
> classLoader.
>
Just the fact to load a Class does, at least on some JVM (yes it hurts)
>
> That would be an implementation bug. If the provider is holding a
> classLoader reference it would prevent the GC on the WAR /
> redeployment no matter what.
>
> I can write a testcase preventing issues. I had written one for
> commons a few years ago.
>
>
>
> What is your other solution?
>
>
>
Either what was there (I know), eviction thread (I know too) or server
integration (I'm fine with that)
>
>
> What is your other solution?
>
Once again "registry" is ok again to start
Re: PR Merge?
Posted by Clebert Suconic <cl...@gmail.com>.
> Not always. Will be true until something else has a reference to the
> classloader, like a provider Class for instance.
> In other words it can just prevent the whole webapp to be garbage collected.
A provider class Is not supposed to hold an explicit reference to the
classLoader.
That would be an implementation bug. If the provider is holding a
classLoader reference it would prevent the GC on the WAR /
redeployment no matter what.
I can write a testcase preventing issues. I had written one for
commons a few years ago.
What is your other solution?
What is your other solution?
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2016-08-23 17:42 GMT+02:00 Clebert Suconic <cl...@gmail.com>:
> > Not sure I get you there, commons clearly doesn't take in charge any of
> that
> > but some containers can force some cleanup (not the other way around) -
> at
> > least for beansutils for instance.
>
> With a WeakHashmap, the cache will be removed once the classLoader is
> released, right? Which is what I am trying to achieve here. The cache
> per classLoader. And Removing the classLoader once the references are
> released.
>
>
Not always. Will be true until something else has a reference to the
classloader, like a provider Class for instance.
In other words it can just prevent the whole webapp to be garbage collected.
>
> >
> > Starting with a "leaking" WeakHashMap<ClassLoader, Provider> and see if
> > consumers are complaining.
> >
> > It would solve your issue and I can make it working in tomee for instance
>
>
> Why it would matter? a WeakHashMap won't cause a leak in anyways. The
> ClassLoader is a weak references. Whenever Tomee released the
> classLoader the element would go away from the WeakHashMap. That's how
> weakHashMaps work, and that's the intent.
>
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
done, will be off the end of the week and back next week but feel free to
test/release etc if you need it ASAP ;)
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://blog-rmannibucau.rhcloud.com> | Old Wordpress Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Tomitriber
<http://www.tomitribe.com> | JavaEE Factory
<https://javaeefactory-rmannibucau.rhcloud.com>
2016-09-12 12:12 GMT+02:00 Romain Manni-Bucau <rm...@gmail.com>:
> will have a look to jsonp now, not sure i'll get enough time to fix it but
> on it for now ;)
>
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://blog-rmannibucau.rhcloud.com> | Old Wordpress Blog
> <http://rmannibucau.wordpress.com> | Github
> <https://github.com/rmannibucau> | LinkedIn
> <https://www.linkedin.com/in/rmannibucau> | Tomitriber
> <http://www.tomitribe.com> | JavaEE Factory
> <https://javaeefactory-rmannibucau.rhcloud.com>
>
> 2016-08-23 19:21 GMT+02:00 Clebert Suconic <cl...@gmail.com>:
>
>> I don't know if I can, but I'm interested on it. I will send an email
>> here if I start something so we won't have double effort around it.
>>
>> So, I ask if anyone started doing this.. please send a heads up here
>> so we won't have 2 people working on the same thing.
>>
>>
>>
>> On Tue, Aug 23, 2016 at 1:17 PM, Romain Manni-Bucau
>> <rm...@gmail.com> wrote:
>> >
>> > 2016-08-23 18:49 GMT+02:00 Mark Struberg <st...@yahoo.de>:
>> >>
>> >> No it's not _that_ easy. Simply using a WeakHashMap wont work. You also
>> >> have to wrap the value in a WeakReference. Otherwise the value in the
>> Map
>> >> will prevent the CL from being garbage collected. A common pitfall
>> I've seen
>> >> soooo often implemented the wrong way ;)
>> >>
>> >
>> > Sounds like a good enough solution to me. JCache impl should be updated
>> as
>> > well probably.
>> >
>> > Who does want to drive this? I'm quite limited in time ATM so happy to
>> let
>> > it be done but can help a bit in 1 or 2 weeks if needed.
>> >
>> >>
>> >> LieGrue,
>> >> strub
>> >>
>> >>
>> >>
>> >>
>> >>
>> >> > On Tuesday, 23 August 2016, 17:42, Clebert Suconic
>> >> > <cl...@gmail.com> wrote:
>> >> > >> Not sure I get you there, commons clearly doesn't take in charge
>> any
>> >> > >> of
>> >> > that
>> >> >> but some containers can force some cleanup (not the other way
>> around)
>> >> >> - at
>> >> >> least for beansutils for instance.
>> >> >
>> >> > With a WeakHashmap, the cache will be removed once the classLoader is
>> >> > released, right? Which is what I am trying to achieve here. The cache
>> >> > per classLoader. And Removing the classLoader once the references are
>> >> >
>> >> > released.
>> >> >
>> >> >
>> >> >>
>> >> >> Starting with a "leaking" WeakHashMap<ClassLoader,
>> >> > Provider> and see if
>> >> >> consumers are complaining.
>> >> >>
>> >> >> It would solve your issue and I can make it working in tomee for
>> >> >> instance
>> >> >
>> >> >
>> >> > Why it would matter? a WeakHashMap won't cause a leak in anyways. The
>> >> > ClassLoader is a weak references. Whenever Tomee released the
>> >> > classLoader the element would go away from the WeakHashMap. That's
>> how
>> >> > weakHashMaps work, and that's the intent.
>> >> >
>> >
>> >
>>
>>
>>
>> --
>> Clebert Suconic
>>
>
>
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
will have a look to jsonp now, not sure i'll get enough time to fix it but
on it for now ;)
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://blog-rmannibucau.rhcloud.com> | Old Wordpress Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Tomitriber
<http://www.tomitribe.com> | JavaEE Factory
<https://javaeefactory-rmannibucau.rhcloud.com>
2016-08-23 19:21 GMT+02:00 Clebert Suconic <cl...@gmail.com>:
> I don't know if I can, but I'm interested on it. I will send an email
> here if I start something so we won't have double effort around it.
>
> So, I ask if anyone started doing this.. please send a heads up here
> so we won't have 2 people working on the same thing.
>
>
>
> On Tue, Aug 23, 2016 at 1:17 PM, Romain Manni-Bucau
> <rm...@gmail.com> wrote:
> >
> > 2016-08-23 18:49 GMT+02:00 Mark Struberg <st...@yahoo.de>:
> >>
> >> No it's not _that_ easy. Simply using a WeakHashMap wont work. You also
> >> have to wrap the value in a WeakReference. Otherwise the value in the
> Map
> >> will prevent the CL from being garbage collected. A common pitfall I've
> seen
> >> soooo often implemented the wrong way ;)
> >>
> >
> > Sounds like a good enough solution to me. JCache impl should be updated
> as
> > well probably.
> >
> > Who does want to drive this? I'm quite limited in time ATM so happy to
> let
> > it be done but can help a bit in 1 or 2 weeks if needed.
> >
> >>
> >> LieGrue,
> >> strub
> >>
> >>
> >>
> >>
> >>
> >> > On Tuesday, 23 August 2016, 17:42, Clebert Suconic
> >> > <cl...@gmail.com> wrote:
> >> > >> Not sure I get you there, commons clearly doesn't take in charge
> any
> >> > >> of
> >> > that
> >> >> but some containers can force some cleanup (not the other way
> around)
> >> >> - at
> >> >> least for beansutils for instance.
> >> >
> >> > With a WeakHashmap, the cache will be removed once the classLoader is
> >> > released, right? Which is what I am trying to achieve here. The cache
> >> > per classLoader. And Removing the classLoader once the references are
> >> >
> >> > released.
> >> >
> >> >
> >> >>
> >> >> Starting with a "leaking" WeakHashMap<ClassLoader,
> >> > Provider> and see if
> >> >> consumers are complaining.
> >> >>
> >> >> It would solve your issue and I can make it working in tomee for
> >> >> instance
> >> >
> >> >
> >> > Why it would matter? a WeakHashMap won't cause a leak in anyways. The
> >> > ClassLoader is a weak references. Whenever Tomee released the
> >> > classLoader the element would go away from the WeakHashMap. That's how
> >> > weakHashMaps work, and that's the intent.
> >> >
> >
> >
>
>
>
> --
> Clebert Suconic
>
Re: PR Merge?
Posted by Clebert Suconic <cl...@gmail.com>.
I don't know if I can, but I'm interested on it. I will send an email
here if I start something so we won't have double effort around it.
So, I ask if anyone started doing this.. please send a heads up here
so we won't have 2 people working on the same thing.
On Tue, Aug 23, 2016 at 1:17 PM, Romain Manni-Bucau
<rm...@gmail.com> wrote:
>
> 2016-08-23 18:49 GMT+02:00 Mark Struberg <st...@yahoo.de>:
>>
>> No it's not _that_ easy. Simply using a WeakHashMap wont work. You also
>> have to wrap the value in a WeakReference. Otherwise the value in the Map
>> will prevent the CL from being garbage collected. A common pitfall I've seen
>> soooo often implemented the wrong way ;)
>>
>
> Sounds like a good enough solution to me. JCache impl should be updated as
> well probably.
>
> Who does want to drive this? I'm quite limited in time ATM so happy to let
> it be done but can help a bit in 1 or 2 weeks if needed.
>
>>
>> LieGrue,
>> strub
>>
>>
>>
>>
>>
>> > On Tuesday, 23 August 2016, 17:42, Clebert Suconic
>> > <cl...@gmail.com> wrote:
>> > >> Not sure I get you there, commons clearly doesn't take in charge any
>> > >> of
>> > that
>> >> but some containers can force some cleanup (not the other way around)
>> >> - at
>> >> least for beansutils for instance.
>> >
>> > With a WeakHashmap, the cache will be removed once the classLoader is
>> > released, right? Which is what I am trying to achieve here. The cache
>> > per classLoader. And Removing the classLoader once the references are
>> >
>> > released.
>> >
>> >
>> >>
>> >> Starting with a "leaking" WeakHashMap<ClassLoader,
>> > Provider> and see if
>> >> consumers are complaining.
>> >>
>> >> It would solve your issue and I can make it working in tomee for
>> >> instance
>> >
>> >
>> > Why it would matter? a WeakHashMap won't cause a leak in anyways. The
>> > ClassLoader is a weak references. Whenever Tomee released the
>> > classLoader the element would go away from the WeakHashMap. That's how
>> > weakHashMaps work, and that's the intent.
>> >
>
>
--
Clebert Suconic
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2016-08-23 18:49 GMT+02:00 Mark Struberg <st...@yahoo.de>:
> No it's not _that_ easy. Simply using a WeakHashMap wont work. You also
> have to wrap the value in a WeakReference. Otherwise the value in the Map
> will prevent the CL from being garbage collected. A common pitfall I've
> seen soooo often implemented the wrong way ;)
>
>
Sounds like a good enough solution to me. JCache impl should be updated as
well probably.
Who does want to drive this? I'm quite limited in time ATM so happy to let
it be done but can help a bit in 1 or 2 weeks if needed.
> LieGrue,
> strub
>
>
>
>
> > On Tuesday, 23 August 2016, 17:42, Clebert Suconic <
> clebert.suconic@gmail.com> wrote:
> > >> Not sure I get you there, commons clearly doesn't take in charge any
> of
> > that
> >> but some containers can force some cleanup (not the other way around)
> - at
> >> least for beansutils for instance.
> >
> > With a WeakHashmap, the cache will be removed once the classLoader is
> > released, right? Which is what I am trying to achieve here. The cache
> > per classLoader. And Removing the classLoader once the references are
> >
> > released.
> >
> >
> >>
> >> Starting with a "leaking" WeakHashMap<ClassLoader,
> > Provider> and see if
> >> consumers are complaining.
> >>
> >> It would solve your issue and I can make it working in tomee for
> instance
> >
> >
> > Why it would matter? a WeakHashMap won't cause a leak in anyways. The
> > ClassLoader is a weak references. Whenever Tomee released the
> > classLoader the element would go away from the WeakHashMap. That's how
> > weakHashMaps work, and that's the intent.
> >
>
Re: PR Merge?
Posted by Mark Struberg <st...@yahoo.de>.
No it's not _that_ easy. Simply using a WeakHashMap wont work. You also have to wrap the value in a WeakReference. Otherwise the value in the Map will prevent the CL from being garbage collected. A common pitfall I've seen soooo often implemented the wrong way ;)
LieGrue,
strub
> On Tuesday, 23 August 2016, 17:42, Clebert Suconic <cl...@gmail.com> wrote:
> >> Not sure I get you there, commons clearly doesn't take in charge any of
> that
>> but some containers can force some cleanup (not the other way around) - at
>> least for beansutils for instance.
>
> With a WeakHashmap, the cache will be removed once the classLoader is
> released, right? Which is what I am trying to achieve here. The cache
> per classLoader. And Removing the classLoader once the references are
>
> released.
>
>
>>
>> Starting with a "leaking" WeakHashMap<ClassLoader,
> Provider> and see if
>> consumers are complaining.
>>
>> It would solve your issue and I can make it working in tomee for instance
>
>
> Why it would matter? a WeakHashMap won't cause a leak in anyways. The
> ClassLoader is a weak references. Whenever Tomee released the
> classLoader the element would go away from the WeakHashMap. That's how
> weakHashMaps work, and that's the intent.
>
Re: PR Merge?
Posted by Clebert Suconic <cl...@gmail.com>.
> Not sure I get you there, commons clearly doesn't take in charge any of that
> but some containers can force some cleanup (not the other way around) - at
> least for beansutils for instance.
With a WeakHashmap, the cache will be removed once the classLoader is
released, right? Which is what I am trying to achieve here. The cache
per classLoader. And Removing the classLoader once the references are
released.
>
> Starting with a "leaking" WeakHashMap<ClassLoader, Provider> and see if
> consumers are complaining.
>
> It would solve your issue and I can make it working in tomee for instance
Why it would matter? a WeakHashMap won't cause a leak in anyways. The
ClassLoader is a weak references. Whenever Tomee released the
classLoader the element would go away from the WeakHashMap. That's how
weakHashMaps work, and that's the intent.
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2016-08-23 17:35 GMT+02:00 Clebert Suconic <cl...@gmail.com>:
> > "Redeployments will remove the old classloader"
> >
> > this is were we "split" I suspect. This is not true since the key is a
> > classloader and classloader is the root of most of everything so GC is
> > rarely enough
> >
>
>
> With a WeakHashMap, the application server should release the
> classLoader once a redeployment occurs. This is how apache-commons do
> in a lot of places. Once a classLoader is released the element of the
> hashmap should go away.
>
>
Not sure I get you there, commons clearly doesn't take in charge any of
that but some containers can force some cleanup (not the other way around)
- at least for beansutils for instance.
>
> >>
> >>
> >> There are serious implications to the application server if that
> >> doesn't work this way. So I doubt any application server would behave
> >> differently.
> >>
> >>
> >> I may be wrong, but this is what I have seen over the years.
> >
> >
> > Think we agree on what we want but disagree on what actually happens
> there.
> >
> > What about starting with the registry I proposed and enhance/fix it on
> > demand? Sounds like a compromise
> >
>
>
> I guess I don't understand your proposal.
>
>
Starting with a "leaking" WeakHashMap<ClassLoader, Provider> and see if
consumers are complaining.
It would solve your issue and I can make it working in tomee for instance
>
> --
> Clebert Suconic
>
Re: PR Merge?
Posted by Clebert Suconic <cl...@gmail.com>.
> "Redeployments will remove the old classloader"
>
> this is were we "split" I suspect. This is not true since the key is a
> classloader and classloader is the root of most of everything so GC is
> rarely enough
>
With a WeakHashMap, the application server should release the
classLoader once a redeployment occurs. This is how apache-commons do
in a lot of places. Once a classLoader is released the element of the
hashmap should go away.
>>
>>
>> There are serious implications to the application server if that
>> doesn't work this way. So I doubt any application server would behave
>> differently.
>>
>>
>> I may be wrong, but this is what I have seen over the years.
>
>
> Think we agree on what we want but disagree on what actually happens there.
>
> What about starting with the registry I proposed and enhance/fix it on
> demand? Sounds like a compromise
>
I guess I don't understand your proposal.
--
Clebert Suconic
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2016-08-23 17:15 GMT+02:00 Clebert Suconic <cl...@gmail.com>:
> On Tue, Aug 23, 2016 at 10:55 AM, Romain Manni-Bucau
> <rm...@gmail.com> wrote:
> >
> > 2016-08-23 16:51 GMT+02:00 Clebert Suconic <cl...@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.
>
> As Far as I know redeployment means a classLoader change. If you cache
> per classLoader a redeployment will remove the old classLoader and add
> a new one.
>
> No applicaition server that I know will change loaded classes once the
> classLoader is up. Redeployments will remove the old classloader and
> create a new one.
>
>
"Redeployments will remove the old classloader"
this is were we "split" I suspect. This is not true since the key is a
classloader and classloader is the root of most of everything so GC is
rarely enough
>
> There are serious implications to the application server if that
> doesn't work this way. So I doubt any application server would behave
> differently.
>
>
> I may be wrong, but this is what I have seen over the years.
>
Think we agree on what we want but disagree on what actually happens there.
What about starting with the registry I proposed and enhance/fix it on
demand? Sounds like a compromise
Re: PR Merge?
Posted by Clebert Suconic <cl...@gmail.com>.
On Tue, Aug 23, 2016 at 10:55 AM, Romain Manni-Bucau
<rm...@gmail.com> wrote:
>
> 2016-08-23 16:51 GMT+02:00 Clebert Suconic <cl...@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.
As Far as I know redeployment means a classLoader change. If you cache
per classLoader a redeployment will remove the old classLoader and add
a new one.
No applicaition server that I know will change loaded classes once the
classLoader is up. Redeployments will remove the old classloader and
create a new one.
There are serious implications to the application server if that
doesn't work this way. So I doubt any application server would behave
differently.
I may be wrong, but this is what I have seen over the years.
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2016-08-23 16:51 GMT+02:00 Clebert Suconic <cl...@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
> <rm...@gmail.com> wrote:
> > Le 21 août 2016 12:24, "Mark Struberg" <st...@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
> >> <rm...@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" <cl...@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" <cl...@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
> >> >>>> <rm...@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
> >> >>>>> > <rm...@gmail.com> wrote:
> >> >>>>> >>
> >> >>>>> >>
> >> >>>>> >>
> >> >>>>> >> 2016-08-19 16:48 GMT+02:00 Mark Struberg <st...@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
> >> >>>>> >>> > <rm...@gmail.com>:
> >> >>>>> >>> >
> >> >>>>> >>> >
> >> >>>>> >>> >
> >> >>>>> >>> > 2016-08-19 15:40 GMT+02:00 Clebert Suconic
> >> >>>>> >>> > <cl...@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
> >> >>>>> >>> > <rm...@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
>
Re: PR Merge?
Posted by Clebert Suconic <cl...@gmail.com>.
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
<rm...@gmail.com> wrote:
> Le 21 août 2016 12:24, "Mark Struberg" <st...@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
>> <rm...@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" <cl...@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 <rm...@gmail.com>
>> >> wrote:
>> >>
>> >>Le 19 août 2016 18:54, "Clebert Suconic" <cl...@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
>> >>>> <rm...@gmail.com> wrote:
>> >>>>>
>> >>>>> Le 19 août 2016 18:06, "Clebert Suconic" <cl...@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
>> >>>>> > <rm...@gmail.com> wrote:
>> >>>>> >>
>> >>>>> >>
>> >>>>> >>
>> >>>>> >> 2016-08-19 16:48 GMT+02:00 Mark Struberg <st...@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
>> >>>>> >>> > <rm...@gmail.com>:
>> >>>>> >>> >
>> >>>>> >>> >
>> >>>>> >>> >
>> >>>>> >>> > 2016-08-19 15:40 GMT+02:00 Clebert Suconic
>> >>>>> >>> > <cl...@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
>> >>>>> >>> > <rm...@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" <jo...@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
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le 21 août 2016 12:24, "Mark Struberg" <st...@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" <cl...@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 <rm...@gmail.com>
wrote:
> >>
> >>Le 19 août 2016 18:54, "Clebert Suconic" <cl...@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 <rm...@gmail.com>
wrote:
> >>>>>
> >>>>> Le 19 août 2016 18:06, "Clebert Suconic" <cl...@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 <st...@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" <jo...@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
> >>
> >>
> >
> >
Re: PR Merge?
Posted by Mark Struberg <st...@yahoo.de>.
We should imo check the spec wording.
If it's not specified in
* the spec PDF
* the official JavaDoc
* the TCK
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.
LieGrue,
strub
On Sunday, 21 August 2016, 10:24, Romain Manni-Bucau <rm...@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" <cl...@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 <rm...@gmail.com> wrote:
>>
>>Le 19 août 2016 18:54, "Clebert Suconic" <cl...@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 <rm...@gmail.com> wrote:
>>>>>
>>>>> Le 19 août 2016 18:06, "Clebert Suconic" <cl...@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 <rm...@gmail.com> wrote:
>>>>> >>
>>>>> >>
>>>>> >>
>>>>> >> 2016-08-19 16:48 GMT+02:00 Mark Struberg <st...@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 <rm...@gmail.com>:
>>>>> >>> >
>>>>> >>> >
>>>>> >>> >
>>>>> >>> > 2016-08-19 15:40 GMT+02:00 Clebert Suconic <cl...@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 <rm...@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" <jo...@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
>>
>>
>
>
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
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" <cl...@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 <rm...@gmail.com>
> wrote:
>
>> Le 19 août 2016 18:54, "Clebert Suconic" <cl...@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 <rm...@gmail.com>
>> wrote:
>> >>
>> >> Le 19 août 2016 18:06, "Clebert Suconic" <cl...@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 <st...@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" <jo...@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
>
>
Re: PR Merge?
Posted by Clebert Suconic <cl...@gmail.com>.
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 <rm...@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
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le 19 août 2016 18:54, "Clebert Suconic" <cl...@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 <rm...@gmail.com>
wrote:
>>
>> Le 19 août 2016 18:06, "Clebert Suconic" <cl...@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 <rm...@gmail.com>
wrote:
>> >>
>> >>
>> >>
>> >> 2016-08-19 16:48 GMT+02:00 Mark Struberg <st...@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" <jo...@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
>
Re: PR Merge?
Posted by Clebert Suconic <cl...@gmail.com>.
Is it really a requirement to change the provider within the same class
loader?
My expectation is to always return the same. When the class loader is gone
the factory itself will be gone.
Or you could cache per class loader. Some sort of weakhashmap<classloader,
provider>.
Notice that there is a separate issue on only using the context
classloader.
On Friday, August 19, 2016, Romain Manni-Bucau <rm...@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
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le 19 août 2016 18:06, "Clebert Suconic" <cl...@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 <rm...@gmail.com>
wrote:
>>
>>
>>
>> 2016-08-19 16:48 GMT+02:00 Mark Struberg <st...@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 <rm...@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" <jo...@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
>
Re: PR Merge?
Posted by Clebert Suconic <cl...@gmail.com>.
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.
On Friday, August 19, 2016, Romain Manni-Bucau <rm...@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
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2016-08-19 16:48 GMT+02:00 Mark Struberg <st...@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 <cl...@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 <rm...@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" <jo...@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
> >
> >
>
>
Re: PR Merge?
Posted by Mark Struberg <st...@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.
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?
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.
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 <rm...@gmail.com>:
>
>
>
> 2016-08-19 15:40 GMT+02:00 Clebert Suconic <cl...@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 <rm...@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" <jo...@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
>
>
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2016-08-19 15:40 GMT+02:00 Clebert Suconic <cl...@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 <rm...@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" <jo...@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
>
>
Re: PR Merge?
Posted by Clebert Suconic <cl...@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.
The performance of having to load the class on every operation is really
heavy.
On Friday, August 19, 2016, Romain Manni-Bucau <rm...@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
Re: PR Merge?
Posted by Romain Manni-Bucau <rm...@gmail.com>.
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" <jo...@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
>