geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Struberg <strub...@yahoo.de>
Subject Re: PR Merge?
Date Sun, 21 Aug 2016 10:24:30 GMT
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 <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
>>
>>
>
>

Mime
View raw message