You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by lu...@apache.org on 2011/05/01 16:29:02 UTC

svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

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;
     }
 }



Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Rene Gielen <gi...@it-neering.net>.
Great, thanks for taking this

On 03.05.11 11:10, Johannes Geppert wrote:
> I will going to fix this.
> 
> https://issues.apache.org/jira/browse/WW-3619
> 
> Johannes
> 
> -----
> 
> --------------
> web: http://www.jgeppert.com
> twitter: http://twitter.com/jogep
> --
> View this message in context: http://struts.1045723.n5.nabble.com/Re-svn-commit-r1098322-struts-sandbox-trunk-struts2-cdi-plugin-src-main-java-org-apache-struts2-cdi-a-tp4366559p4366770.html
> Sent from the Struts - Dev mailing list archive at Nabble.com.
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
> 

-- 
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-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Johannes Geppert <jo...@apache.org>.
I will going to fix this.

https://issues.apache.org/jira/browse/WW-3619

Johannes

-----

--------------
web: http://www.jgeppert.com
twitter: http://twitter.com/jogep
--
View this message in context: http://struts.1045723.n5.nabble.com/Re-svn-commit-r1098322-struts-sandbox-trunk-struts2-cdi-plugin-src-main-java-org-apache-struts2-cdi-a-tp4366559p4366770.html
Sent from the Struts - Dev mailing list archive at Nabble.com.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Rene Gielen <gi...@it-neering.net>.
I'd even wrap WARN - sysops might configure to just log ERROR and FATAL :)

On 03.05.11 10:00, Johannes Geppert wrote:
> I also agree to René, we should wrap all debug and info messages.
> 
> Johannes
> 
> -----
> 
> --------------
> web: http://www.jgeppert.com
> twitter: http://twitter.com/jogep
> --
> View this message in context: http://struts.1045723.n5.nabble.com/Re-svn-commit-r1098322-struts-sandbox-trunk-struts2-cdi-plugin-src-main-java-org-apache-struts2-cdi-a-tp4366559p4366635.html
> Sent from the Struts - Dev mailing list archive at Nabble.com.
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
> 

-- 
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-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Johannes Geppert <jo...@apache.org>.
I also agree to René, we should wrap all debug and info messages.

Johannes

-----

--------------
web: http://www.jgeppert.com
twitter: http://twitter.com/jogep
--
View this message in context: http://struts.1045723.n5.nabble.com/Re-svn-commit-r1098322-struts-sandbox-trunk-struts2-cdi-plugin-src-main-java-org-apache-struts2-cdi-a-tp4366559p4366635.html
Sent from the Struts - Dev mailing list archive at Nabble.com.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Maurizio Cucchiara <ma...@gmail.com>.
I totally agree with René, in this way you pay the cost of string
conversion/concatenation.

On 3 May 2011 09:07, Rene Gielen <gi...@it-neering.net> wrote:
> 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, lukaszlenart@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-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>



-- 
Maurizio Cucchiara

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Lukasz Lenart <lu...@googlemail.com>.
It'd be better to not overuse logging, that will improve performance
and readability. We'd take a closer look how we use logging, as it was
mentioned on the last Devoxx - "logging should be threaded in the same
way as UI for users" (more or less ;-) )

And keeping our own Logger is the best option, then we can always
change underlaying implementation (slf4j, log4j, etc)


Kind regards
-- 
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/
Warszawa JUG conference - Confitura http://confitura.pl/


2011/5/6 Rene Gielen <gi...@it-neering.net>:
> See inline comments (initial mail wrongly addressed)
>
> On 05.05.11 07:02, Lukasz Lenart wrote:
>> 2011/5/5 Martin Cooper <ma...@apache.org>:
>>> On Wed, May 4, 2011 at 9:32 PM, Lukasz Lenart
>>> <lu...@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());
>>
>> log.info("And the answer is: " + DO_SOMETHING_CONSTANT_STRING) will be
>> optimized by JIT and that's why I've removed logging gates. There
>> wasn't any call to a method and object concatenation.
>>
>>> 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());
>>>    }
>>>
>>
>> Yes, I agree, but it wan't the case.
>>
>>> 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.
>>
>> That's why I prefer a code review like this and discussion over it
>> instead of a common rule that we should blindly follow. Because rule
>> don't teach how to be a good programmer, discussion do. And if someone
>> did something that can impact performance, asking and explaining is
>> far better, than saying we have a rule. Because quite often what was
>> true for Java 1.4, isn't for 1.5 any more.
>>
>
> I'm with you on readability.
>
> I would agree with you regarding reviews if we had some mandatory code
> review mechanism in place, which is not the case. Actually I stumbled by
> accident about this commit, and I doubt that each commit gets reviewed
> either intensively enough or even at all. I've seen far to many
> programmers not knowing about parameter evaluation costs, and while we
> create some awareness here with this discussion, I'm sure it will be
> mostly forgot0ten six months from now :)
>
> If a "not so aware" committer would come across those guards all over
> the code base, I would tend to think that he'd either be lazy, by just
> adding his additional code within an existing guarded log statement, or
> ask in the dev list why he found all this crazy code that seems to be
> intentional.
>
> Personally, if I had to chose between improved readability and improved
> performance, I'd rather drop some readability...
>
> - René
>
> --
> 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-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Rene Gielen <gi...@it-neering.net>.
See inline comments (initial mail wrongly addressed)

On 05.05.11 07:02, Lukasz Lenart wrote:
> 2011/5/5 Martin Cooper <ma...@apache.org>:
>> On Wed, May 4, 2011 at 9:32 PM, Lukasz Lenart
>> <lu...@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());
> 
> log.info("And the answer is: " + DO_SOMETHING_CONSTANT_STRING) will be
> optimized by JIT and that's why I've removed logging gates. There
> wasn't any call to a method and object concatenation.
> 
>> 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());
>>    }
>>
> 
> Yes, I agree, but it wan't the case.
> 
>> 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.
> 
> That's why I prefer a code review like this and discussion over it
> instead of a common rule that we should blindly follow. Because rule
> don't teach how to be a good programmer, discussion do. And if someone
> did something that can impact performance, asking and explaining is
> far better, than saying we have a rule. Because quite often what was
> true for Java 1.4, isn't for 1.5 any more.
> 

I'm with you on readability.

I would agree with you regarding reviews if we had some mandatory code
review mechanism in place, which is not the case. Actually I stumbled by
accident about this commit, and I doubt that each commit gets reviewed
either intensively enough or even at all. I've seen far to many
programmers not knowing about parameter evaluation costs, and while we
create some awareness here with this discussion, I'm sure it will be
mostly forgot0ten six months from now :)

If a "not so aware" committer would come across those guards all over
the code base, I would tend to think that he'd either be lazy, by just
adding his additional code within an existing guarded log statement, or
ask in the dev list why he found all this crazy code that seems to be
intentional.

Personally, if I had to chose between improved readability and improved
performance, I'd rather drop some readability...

- René

-- 
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-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Lukasz Lenart <lu...@googlemail.com>.
2011/5/5 Martin Cooper <ma...@apache.org>:
> On Wed, May 4, 2011 at 9:32 PM, Lukasz Lenart
> <lu...@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());

log.info("And the answer is: " + DO_SOMETHING_CONSTANT_STRING) will be
optimized by JIT and that's why I've removed logging gates. There
wasn't any call to a method and object concatenation.

> 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());
>    }
>

Yes, I agree, but it wan't the case.

> 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.

That's why I prefer a code review like this and discussion over it
instead of a common rule that we should blindly follow. Because rule
don't teach how to be a good programmer, discussion do. And if someone
did something that can impact performance, asking and explaining is
far better, than saying we have a rule. Because quite often what was
true for Java 1.4, isn't for 1.5 any more.


Regards
-- 
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/
Warszawa JUG conference - Confitura http://confitura.pl/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Martin Cooper <ma...@apache.org>.
On Wed, May 4, 2011 at 9:32 PM, Lukasz Lenart
<lu...@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 <gi...@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, lukaszlenart@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-unsubscribe@struts.apache.org
>> For additional commands, e-mail: dev-help@struts.apache.org
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Lukasz Lenart <lu...@googlemail.com>.
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 ;-) )

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 <gi...@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, lukaszlenart@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-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Lukasz Lenart <lu...@googlemail.com>.
2011/5/4 Johannes Geppert <jo...@apache.org>:
> Make Struts2 independent from a specific log implementation is a nice goal.

We can simply extend current Logger class and add varargs versions of
the logging methods and the old ones mark as @Deprecated.


Kind regards
-- 
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/
Warszawa JUG conference - Confitura http://confitura.pl/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Johannes Geppert <jo...@apache.org>.
Make Struts2 independent from a specific log implementation is a nice goal.

+1

Johannes


Rene Gielen wrote:
> 
> There is another possible solution / improvement we might want to think
> of: Switch our logging system to slf4j facade logging, which addresses -
> besides a lot of other cool things - the parameter evaluation issue with
> varags and late concatenation.
> 
> See:
> http://www.slf4j.org/
> http://www.slf4j.org/faq.html
> 


-----

--------------
web: http://www.jgeppert.com
twitter: http://twitter.com/jogep
--
View this message in context: http://struts.1045723.n5.nabble.com/Re-svn-commit-r1098322-struts-sandbox-trunk-struts2-cdi-plugin-src-main-java-org-apache-struts2-cdi-a-tp4366559p4369135.html
Sent from the Struts - Dev mailing list archive at Nabble.com.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Steven Benitez <st...@gmail.com>.
+1 for slf4j

On Tue, May 3, 2011 at 4:23 AM, Rene Gielen <gi...@it-neering.net> wrote:

> There is another possible solution / improvement we might want to think
> of: Switch our logging system to slf4j facade logging, which addresses -
> besides a lot of other cool things - the parameter evaluation issue with
> varags and late concatenation.
>
> See:
> http://www.slf4j.org/
> http://www.slf4j.org/faq.html
>
> On 03.05.11 09:07, Rene Gielen wrote:
> > 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, lukaszlenart@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-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>

Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Rene Gielen <gi...@it-neering.net>.
There is another possible solution / improvement we might want to think
of: Switch our logging system to slf4j facade logging, which addresses -
besides a lot of other cool things - the parameter evaluation issue with
varags and late concatenation.

See:
http://www.slf4j.org/
http://www.slf4j.org/faq.html

On 03.05.11 09:07, Rene Gielen wrote:
> 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, lukaszlenart@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-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Re: svn commit: r1098322 - /struts/sandbox/trunk/struts2-cdi-plugin/src/main/java/org/apache/struts2/cdi/CdiObjectFactory.java

Posted by Rene Gielen <gi...@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, lukaszlenart@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-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org