On Wed, May 4, 2011 at 9:32 PM, Lukasz Lenart
<lukasz.len...@googlemail.com> wrote:
> Hi,
>
> I've removed the parts where concatenating was over constants -
> constant message plus constant as a string. I'm pretty sure that the
> modern JIT compilers would optimize that as well. And the code is more
> readable IMHO.
> Another thing, debug(), info(), etc checks if the given log level is
> set up or not and then performs logging (IO request, sending e-mail,
> etc). So, it will not affect performance as well (except if there is a
> bug ;-) )
Not true. In order to call the method in the first place, all of the
arguments must be evaluated.
If you do this, the expensive function will _always_ be invoked,
whether 'info' level logging is enabled or not:
log.info("And the answer is: " + doSomethingExpensive());
However, if you guard it, like this, the expensive function is _only_
evaluated when the guard is passed:
if (log.isInfoEnabled()) {
log.info("And the answer is: " + doSomethingExpensive());
}
The usual argument I've seen for getting rid of the guards is that
most of the calls don't do anything expensive, so there's little
reason to guard them. However, it's far too easy for someone else to
come along later and modify the log message without thinking about it
perhaps now needing a guard, because it's now more expensive than it
used to be. Better, then, to always guard rather than kill performance
later by accident.
--
Martin Cooper
> I agree, if we're using more complicated statements, we should
> consider to use or not logging gates, but it shouldn't be a common
> pattern. Maybe less logging would be better instead.
>
> I'm thinking to remove some other parts, like this for example:
>
> LOG.info("[findBeanManager]: Checking for BeanManager under JNDI key "
> + CDI_JNDIKEY_BEANMANAGER_COMP);
>
> It's just a useless information, by spec (CDI and Weld), BM have to be
> under two keys and if we've checked both and didn't find it, we should
> log message like this:
>
> LOG.error("Could not get BeanManager from JNDI context, checked under
> ["+CDI_JNDIKEY_BEANMANAGER_COMP+"] and
> ["+CDI_JNDIKEY_BEANMANAGER_APP+"], e);
>
> From a user perspective this's the most informative message.
> Performance shouldn't be the only factor here.
>
>
> Regards
> --
> Łukasz
> + 48 606 323 122 http://www.lenart.org.pl/
> Warszawa JUG conference - Confitura http://confitura.pl/
>
>
> 2011/5/3 Rene Gielen <gie...@it-neering.net>:
>> Lukasz,
>>
>> why were you removing the logging gates, aka the if blocks around log
>> statements? I agree that for simple String parameters (without
>> concatenation) they might be left out, but for everything else they
>> really help a lot for performance - that's why I use them in general.
>>
>> We should even consider reviewing S2 code if there are more unguarded
>> log statements, to squeeze out the last bit of performance :)
>>
>> See also http://logging.apache.org/log4j/1.2/faq.html#a2.3
>>
>> - René
>>
>> On 01.05.11 16:29, lukaszlen...@apache.org wrote:
>>> Author: lukaszlenart
>>> Date: Sun May 1 14:29:02 2011
>>> New Revision: 1098322
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1098322&view=rev
>>> Log:
>>> Removes unnecessary checking for log level and uses ConcurrentHashMap
>>> instead of HashMap
>>>
>>> Modified:
>>>
>>> struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java
>>>
>>> Modified:
>>> struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java
>>> URL:
>>> http://svn.apache.org/viewvc/struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java?rev=1098322&r1=1098321&r2=1098322&view=diff
>>> ==============================================================================
>>> ---
>>> struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java
>>> (original)
>>> +++
>>> struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java
>>> Sun May 1 14:29:02 2011
>>> @@ -23,14 +23,14 @@ import com.opensymphony.xwork2.ObjectFac
>>> import com.opensymphony.xwork2.util.logging.Logger;
>>> import com.opensymphony.xwork2.util.logging.LoggerFactory;
>>>
>>> +import javax.enterprise.context.spi.CreationalContext;
>>> import javax.enterprise.inject.spi.BeanManager;
>>> import javax.enterprise.inject.spi.InjectionTarget;
>>> -import javax.enterprise.context.spi.CreationalContext;
>>> import javax.naming.Context;
>>> import javax.naming.InitialContext;
>>> import javax.naming.NamingException;
>>> import java.util.Map;
>>> -import java.util.HashMap;
>>> +import java.util.concurrent.ConcurrentHashMap;
>>>
>>> /**
>>> * CdiObjectFactory allows Struts 2 managed objects, like Actions,
>>> Interceptors or Results, to be injected by a Contexts
>>> @@ -52,7 +52,7 @@ public class CdiObjectFactory extends Ob
>>> protected BeanManager beanManager;
>>> protected CreationalContext ctx;
>>>
>>> - Map<Class<?>, InjectionTarget<?>> injectionTargetCache = new
>>> HashMap<Class<?>, InjectionTarget<?>>();
>>> + Map<Class<?>, InjectionTarget<?>> injectionTargetCache = new
>>> ConcurrentHashMap<Class<?>, InjectionTarget<?>>();
>>>
>>> public CdiObjectFactory() {
>>> super();
>>> @@ -76,25 +76,17 @@ public class CdiObjectFactory extends Ob
>>> BeanManager bm;
>>> try {
>>> Context initialContext = new InitialContext();
>>> - if (LOG.isInfoEnabled()) {
>>> - LOG.info("[findBeanManager]: Checking for BeanManager
>>> under JNDI key " + CDI_JNDIKEY_BEANMANAGER_COMP);
>>> - }
>>> + LOG.info("[findBeanManager]: Checking for BeanManager under
>>> JNDI key " + CDI_JNDIKEY_BEANMANAGER_COMP);
>>> try {
>>> bm = (BeanManager)
>>> initialContext.lookup(CdiObjectFactory.CDI_JNDIKEY_BEANMANAGER_COMP);
>>> - } catch ( NamingException e ) {
>>> - if (LOG.isWarnEnabled()) {
>>> - LOG.warn("[findBeanManager]: Lookup failed.");
>>> - }
>>> - if (LOG.isInfoEnabled()) {
>>> - LOG.info("[findBeanManager]: Checking for BeanManager
>>> under JNDI key " + CDI_JNDIKEY_BEANMANAGER_APP);
>>> - }
>>> + } catch (NamingException e) {
>>> + LOG.warn("[findBeanManager]: Lookup failed.", e);
>>> + LOG.info("[findBeanManager]: Checking for BeanManager
>>> under JNDI key " + CDI_JNDIKEY_BEANMANAGER_APP);
>>> bm = (BeanManager)
>>> initialContext.lookup(CdiObjectFactory.CDI_JNDIKEY_BEANMANAGER_APP);
>>> }
>>> - if (LOG.isInfoEnabled()) {
>>> - LOG.info("[findBeanManager]: BeanManager found.");
>>> - }
>>> + LOG.info("[findBeanManager]: BeanManager found.");
>>> return bm;
>>> - } catch ( NamingException e ) {
>>> + } catch (NamingException e) {
>>> LOG.error("Could not get BeanManager from JNDI context", e);
>>> }
>>> return null;
>>> @@ -102,7 +94,7 @@ public class CdiObjectFactory extends Ob
>>>
>>> @Override
>>> @SuppressWarnings("unchecked")
>>> - public Object buildBean( String className, Map<String, Object>
>>> extraContext, boolean injectInternal )
>>> + public Object buildBean(String className, Map<String, Object>
>>> extraContext, boolean injectInternal)
>>> throws Exception {
>>>
>>> Class<?> clazz = getClassInstance(className);
>>> @@ -124,19 +116,16 @@ public class CdiObjectFactory extends Ob
>>> * will be created.
>>> *
>>> * @param clazz The class to get a InjectionTarget instance for.
>>> - *
>>> * @return if found in cache, an existing instance. A new instance
>>> otherwise.
>>> */
>>> - protected InjectionTarget<?> getInjectionTarget( Class<?> clazz ) {
>>> + protected InjectionTarget<?> getInjectionTarget(Class<?> clazz) {
>>> InjectionTarget<?> result;
>>> - synchronized ( this ) {
>>> - result = injectionTargetCache.get(clazz);
>>> - if (result == null) {
>>> - result =
>>> beanManager.createInjectionTarget(beanManager.createAnnotatedType(clazz));
>>> - injectionTargetCache.put(clazz, result);
>>> - }
>>> -
>>> + result = injectionTargetCache.get(clazz);
>>> + if (result == null) {
>>> + result =
>>> beanManager.createInjectionTarget(beanManager.createAnnotatedType(clazz));
>>> + injectionTargetCache.put(clazz, result);
>>> }
>>> +
>>> return result;
>>> }
>>>
>>> @@ -144,11 +133,10 @@ public class CdiObjectFactory extends Ob
>>> * Simple wrapper for CreationalContext creation.
>>> *
>>> * @param beanManager the BeanManager to use for creating the context.
>>> - *
>>> * @return the context to use, if given BeanManager was not
>>> <tt>null</tt>. <tt>null</tt> otherwise.
>>> */
>>> @SuppressWarnings("unchecked")
>>> - protected CreationalContext buildNonContextualCreationalContext(
>>> BeanManager beanManager ) {
>>> + protected CreationalContext
>>> buildNonContextualCreationalContext(BeanManager beanManager) {
>>> return beanManager != null ?
>>> beanManager.createCreationalContext(null) : null;
>>> }
>>> }
>>>
>>>
>>
>> --
>> René Gielen
>> IT-Neering.net
>> Saarstrasse 100, 52062 Aachen, Germany
>> Tel: +49-(0)241-4010770
>> Fax: +49-(0)241-4010771
>> Cel: +49-(0)163-2844164
>> http://twitter.com/rgielen
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscr...@struts.apache.org
>> For additional commands, e-mail: dev-h...@struts.apache.org
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@struts.apache.org
> For additional commands, e-mail: dev-h...@struts.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@struts.apache.org
For additional commands, e-mail: dev-h...@struts.apache.org
