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