JCgH4164838Gh792C124B5 commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359618422
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlLRUCache.java:
##########
@@ -36,15 +36,15 @@
public class OgnlLRUCache<Key, Value> implements OgnlCache<Key, Value> {
private final Map<Key, Value> ognlLRUCache;
- private final AtomicInteger cacheEvictionLimit = new AtomicInteger(2500);
+ private final AtomicInteger cacheEvictionLimit = new AtomicInteger();
Review Comment:
The `OgnlDefaultCache` comment above would apply here as well.
##########
core/src/main/resources/org/apache/struts2/default.properties:
##########
@@ -243,25 +243,25 @@ struts.ognl.enableExpressionCache=true
### For expressionCacheLRUMode true, the limit will ensure the cache does not
exceed
### that size, dropping the oldest (least-recently-used) expressions to add
new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.expressionCacheMaxSize=1000
+struts.ognl.expressionCacheMaxSize=10000
### Indicates if the OGNL expressionCache should use LRU mode.
### NOTE: When true, make sure to set the expressionCacheMaxSize to a
reasonable value
### for your application. Otherwise the default limit will never
(practically) be reached.
-# struts.ognl.expressionCacheLRUMode=false
+struts.ognl.expressionCacheLRUMode=true
Review Comment:
@ben-manes already pointed out potential implications in making the LRU mode
cache active by default (switching from a concurrent to synchronized cache).
It might be safer to keep LRU mode caching off by default, at least until
someone can confirm if performance of the LRU mode cache is acceptable under
load (benchmarking).
##########
core/src/main/resources/org/apache/struts2/default.properties:
##########
@@ -243,25 +243,25 @@ struts.ognl.enableExpressionCache=true
### For expressionCacheLRUMode true, the limit will ensure the cache does not
exceed
### that size, dropping the oldest (least-recently-used) expressions to add
new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.expressionCacheMaxSize=1000
+struts.ognl.expressionCacheMaxSize=10000
Review Comment:
If an (uncommented) initial value it to be used/set in `default.properties`,
maybe it should match the programmatic defaults in the factory and cache
instances ?
Whether 10000 (or 25000) is a reasonable default might not be determined
without some benchmarks.
This comment would apply to any of the numeric size choices in the
properties file.
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -705,29 +710,31 @@ public void copy(final Object from, final Object to,
final Map<String, Object> c
}
for (PropertyDescriptor fromPd : fromPds) {
Review Comment:
Looks reasonable to me.
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -673,9 +673,15 @@ public void copy(final Object from, final Object to, final
Map<String, Object> c
* note if exclusions AND inclusions are supplied and
not null nothing will get copied.
* @param editable the class (or interface) to restrict property setting to
*/
- public void copy(final Object from, final Object to, final Map<String,
Object> context, Collection<String> exclusions, Collection<String> inclusions,
Class<?> editable) {
+ public void copy(final Object from,
+ final Object to,
+ final Map<String, Object> context,
+ Collection<String> exclusions,
+ Collection<String> inclusions,
+ Class<?> editable) {
if (from == null || to == null) {
- LOG.warn("Attempting to copy from or to a null source. This is
illegal and is bein skipped. This may be due to an error in an OGNL expression,
action chaining, or some other event.");
+ LOG.warn(
Review Comment:
It looks like the wording of that log warning has been invariant as far back
as the history for the file goes. Does changing it by reducing the wording
detail (other than maybe fixing the "is bein" typo) make it less clear ?
I am unsure of how frequently this log statement might get triggered, but
adding "`new RuntimeException()`" as a parameter would (I think ?) result in a
stack-trace being output each time. That might potentially create a lot of log
clutter (a trade-off, if the stack-trace gives better context) ?
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlDefaultCache.java:
##########
@@ -30,10 +30,10 @@
public class OgnlDefaultCache<Key, Value> implements OgnlCache<Key, Value> {
private final ConcurrentHashMap<Key, Value> ognlCache;
- private final AtomicInteger cacheEvictionLimit = new AtomicInteger(25000);
+ private final AtomicInteger cacheEvictionLimit = new AtomicInteger();
Review Comment:
I think setting a positive non-zero value for the initializer was to avoid
the instance ever having a `cacheEvictionLimit` of zero (which "`new
AtomicInteger()`" would). Also, if someone built a custom cache implementation
extending `OgnlDefaultCache` with an overridden constructor that did not call
`super()`, a (hopefully) reasonable nonzero initial value would be ensured.
##########
core/src/main/resources/org/apache/struts2/default.properties:
##########
@@ -243,25 +243,25 @@ struts.ognl.enableExpressionCache=true
### For expressionCacheLRUMode true, the limit will ensure the cache does not
exceed
### that size, dropping the oldest (least-recently-used) expressions to add
new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.expressionCacheMaxSize=1000
+struts.ognl.expressionCacheMaxSize=10000
### Indicates if the OGNL expressionCache should use LRU mode.
### NOTE: When true, make sure to set the expressionCacheMaxSize to a
reasonable value
### for your application. Otherwise the default limit will never
(practically) be reached.
-# struts.ognl.expressionCacheLRUMode=false
+struts.ognl.expressionCacheLRUMode=true
### Specify a limit to the number of entries in the OGNL beanInfoCache.
### For the standard beanInfoCache mode, when the limit is exceeded the entire
cache's
### content will be cleared (can help prevent memory leaks).
### For beanInfoCacheLRUMode true, the limit will ensure the cache does not
exceed
### that size, dropping the oldest (least-recently-used) expressions to add
new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.beanInfoCacheMaxSize=1000
+struts.ognl.beanInfoCacheMaxSize=5000
### Indicates if the OGNL beanInfoCache should use LRU mode.
### NOTE: When true, make sure to set the beanInfoCacheMaxSize to a reasonable
value
### for your application. Otherwise the default limit will never
(practically) be reached.
-# struts.ognl.beanInfoCacheLRUMode=false
+struts.ognl.beanInfoCacheLRUMode=true
Review Comment:
Above comment on LRU mode defaults applies here as well.
##########
core/src/main/resources/org/apache/struts2/default.properties:
##########
@@ -243,25 +243,25 @@ struts.ognl.enableExpressionCache=true
### For expressionCacheLRUMode true, the limit will ensure the cache does not
exceed
### that size, dropping the oldest (least-recently-used) expressions to add
new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.expressionCacheMaxSize=1000
+struts.ognl.expressionCacheMaxSize=10000
### Indicates if the OGNL expressionCache should use LRU mode.
### NOTE: When true, make sure to set the expressionCacheMaxSize to a
reasonable value
### for your application. Otherwise the default limit will never
(practically) be reached.
-# struts.ognl.expressionCacheLRUMode=false
+struts.ognl.expressionCacheLRUMode=true
### Specify a limit to the number of entries in the OGNL beanInfoCache.
### For the standard beanInfoCache mode, when the limit is exceeded the entire
cache's
### content will be cleared (can help prevent memory leaks).
### For beanInfoCacheLRUMode true, the limit will ensure the cache does not
exceed
### that size, dropping the oldest (least-recently-used) expressions to add
new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.beanInfoCacheMaxSize=1000
+struts.ognl.beanInfoCacheMaxSize=5000
Review Comment:
Above comment for numeric defaults applies here as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@struts.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
