[infinispan-dev] BeanUtils.getterMethod

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[infinispan-dev] BeanUtils.getterMethod

Sanne Grinovero-3
Hello all,
could anybody explain why the ComponentRegistry is performing the
following operation ?

The method
AbstractComponentRegistry#getFromConfiguration(Class<T>)

which invokes:
org.infinispan.util.BeanUtils.getterMethod(Class, Class)

and this one throws thousands of NoSuchMethodException if you have a
rather large number of caches to see if it can find a matching
element, and seems to slow down startup significantly in some cases.

Throwing such an exception seems to be expected since it's inspecting
Configuration.class (statically!) - it always returns null.
I've tried to remove all those methods and short-circuit a nicely
performing "return null": not a single test failed.

(No test failed -> useless code, or missing tests)

Shall I remove the code?

Cheers,
Sanne
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] BeanUtils.getterMethod

Mircea Markus

On 3 Aug 2011, at 19:39, Sanne Grinovero wrote:

> Hello all,
> could anybody explain why the ComponentRegistry is performing the
> following operation ?
>
> The method
> AbstractComponentRegistry#getFromConfiguration(Class<T>)
>
> which invokes:
> org.infinispan.util.BeanUtils.getterMethod(Class, Class)
>
> and this one throws thousands of NoSuchMethodException if you have a
> rather large number of caches to see if it can find a matching
> element, and seems to slow down startup significantly in some cases.
>
> Throwing such an exception seems to be expected since it's inspecting
> Configuration.class (statically!) - it always returns null.
> I've tried to remove all those methods and short-circuit a nicely
> performing "return null": not a single test failed.
sounds sensible to me: this is not public API and that method's logic is pretty straight forward. Of course we can start a discussion on IRC about this if you want...
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] BeanUtils.getterMethod

Sanne Grinovero-3
> sounds sensible to me: this is not public API and that method's logic is pretty straight forward. Of course we can start a discussion on IRC about this if you want...

Not sure I understand you. You mean it makes sense to remove the code,
or the code makes sense to you so I'm missing something?

IRC won't work since you're not online :)

Cheers,
Sanne
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] BeanUtils.getterMethod

Dan Berindei
I think it's supposed to be an automatic version of
https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/config/GlobalConfiguration.java#L326
I prefer the manual version in GlobalConfiguration, although I don't
think there are any users for those components either.

Dan


On Wed, Aug 3, 2011 at 10:59 PM, Sanne Grinovero <[hidden email]> wrote:

>> sounds sensible to me: this is not public API and that method's logic is pretty straight forward. Of course we can start a discussion on IRC about this if you want...
>
> Not sure I understand you. You mean it makes sense to remove the code,
> or the code makes sense to you so I'm missing something?
>
> IRC won't work since you're not online :)
>
> Cheers,
> Sanne
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] BeanUtils.getterMethod

Mircea Markus
In reply to this post by Sanne Grinovero-3

On 3 Aug 2011, at 20:59, Sanne Grinovero wrote:

>> sounds sensible to me: this is not public API and that method's logic is pretty straight forward. Of course we can start a discussion on IRC about this if you want...
>
> Not sure I understand you. You mean it makes sense to remove the code
to remove the code.
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] BeanUtils.getterMethod

Sanne Grinovero-3
In reply to this post by Dan Berindei
2011/8/3 Dan Berindei <[hidden email]>:
> I think it's supposed to be an automatic version of
> https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/config/GlobalConfiguration.java#L326
> I prefer the manual version in GlobalConfiguration, although I don't
> think there are any users for those components either.
>
> Dan

I was thinking something similar, but would like to know if it's still
used, somebody must know about this code?
Initially I thought it was some clever way to extract instances from a
user-customizable configuration instance, but since it's reflecting on
the static definition of Configuration it doesn't look like it's meant
to support extensions of it.

If it's used we could cache the references to existing methods and
track also which methods don't exist, so we limit the number of
exceptions to a single one per attempted method name (i.e. starting
500 caches would cost as starting one).
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] BeanUtils.getterMethod

Dan Berindei
On Thu, Aug 4, 2011 at 2:03 AM, Sanne Grinovero <[hidden email]> wrote:

> 2011/8/3 Dan Berindei <[hidden email]>:
>> I think it's supposed to be an automatic version of
>> https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/config/GlobalConfiguration.java#L326
>> I prefer the manual version in GlobalConfiguration, although I don't
>> think there are any users for those components either.
>>
>> Dan
>
> I was thinking something similar, but would like to know if it's still
> used, somebody must know about this code?
> Initially I thought it was some clever way to extract instances from a
> user-customizable configuration instance, but since it's reflecting on
> the static definition of Configuration it doesn't look like it's meant
> to support extensions of it.
>
> If it's used we could cache the references to existing methods and
> track also which methods don't exist, so we limit the number of
> exceptions to a single one per attempted method name (i.e. starting
> 500 caches would cost as starting one).

Even a custom Configuration could register its objects in the
component registry manually in an @Inject method, like
GlobalConfiguration does. But I don't see any good reason for
injecting bits of the configuration instead of injecting the whole
thing and navigating the configuration tree in the component code.

So I'd go with your initial suggestion and remove it.

Dan

> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] BeanUtils.getterMethod

Manik Surtani
In reply to this post by Sanne Grinovero-3
Sorry for jumping in on this so late.

The purpose behind this is so that if someone were to set a component directly in the configuration it can be retrieved.  E.g., setting a TransactionManager using cfg.setTransactionManager() rather than setting a transaction manager lookup.  

But this is legacy since these setters and getters don't exist anymore.  I think it is safe to remove the check in the abstract component registry and subsequently the BeanUtils method.

I'll issue a pull req in a bit.

Cheers
Manik

On 3 Aug 2011, at 19:39, Sanne Grinovero wrote:

> Hello all,
> could anybody explain why the ComponentRegistry is performing the
> following operation ?
>
> The method
> AbstractComponentRegistry#getFromConfiguration(Class<T>)
>
> which invokes:
> org.infinispan.util.BeanUtils.getterMethod(Class, Class)
>
> and this one throws thousands of NoSuchMethodException if you have a
> rather large number of caches to see if it can find a matching
> element, and seems to slow down startup significantly in some cases.
>
> Throwing such an exception seems to be expected since it's inspecting
> Configuration.class (statically!) - it always returns null.
> I've tried to remove all those methods and short-circuit a nicely
> performing "return null": not a single test failed.
>
> (No test failed -> useless code, or missing tests)
>
> Shall I remove the code?
>
> Cheers,
> Sanne
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

--
Manik Surtani
[hidden email]
twitter.com/maniksurtani

Lead, Infinispan
http://www.infinispan.org




_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev