Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WW-5355 Integrate W-TinyLfu cache and use by default #766

Merged
merged 18 commits into from
Oct 18, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,11 @@
<artifactId>freemarker</artifactId>
</dependency>

<dependency>
<groupId>com.github.ben-manes.caffeine</groupId>
<artifactId>caffeine</artifactId>
</dependency>

<dependency>
<groupId>javax.servlet</groupId>
<artifactId>javax.servlet-api</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
import com.opensymphony.xwork2.ognl.DefaultOgnlBeanInfoCacheFactory;
import com.opensymphony.xwork2.ognl.DefaultOgnlExpressionCacheFactory;
import com.opensymphony.xwork2.ognl.ExpressionCacheFactory;
import com.opensymphony.xwork2.ognl.OgnlCacheFactory;
import com.opensymphony.xwork2.ognl.OgnlReflectionProvider;
import com.opensymphony.xwork2.ognl.OgnlUtil;
import com.opensymphony.xwork2.ognl.OgnlValueStackFactory;
Expand Down Expand Up @@ -109,6 +110,7 @@

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -125,6 +127,24 @@
*/
public class DefaultConfiguration implements Configuration {

public static final Map<String, Object> BOOTSTRAP_CONSTANTS;
Copy link
Member Author

@kusalk kusalk Oct 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted bootstrap constants into a public static final field for reuse by the default Struts configuration provider and any test code as needed. Adding new required constants was previously a pain


static {
Map<String, Object> constants = new HashMap<>();
constants.put(StrutsConstants.STRUTS_DEVMODE, Boolean.FALSE);
constants.put(StrutsConstants.STRUTS_OGNL_LOG_MISSING_PROPERTIES, Boolean.FALSE);
constants.put(StrutsConstants.STRUTS_OGNL_ENABLE_EVAL_EXPRESSION, Boolean.FALSE);
constants.put(StrutsConstants.STRUTS_OGNL_ENABLE_EXPRESSION_CACHE, Boolean.TRUE);
constants.put(StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, Boolean.FALSE);
constants.put(StrutsConstants.STRUTS_I18N_RELOAD, Boolean.FALSE);
constants.put(StrutsConstants.STRUTS_MATCHER_APPEND_NAMED_PARAMETERS, Boolean.TRUE);
constants.put(StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_TYPE, OgnlCacheFactory.CacheType.CAFFEINE_WTLFU);
constants.put(StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE, 10000);
constants.put(StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_TYPE, OgnlCacheFactory.CacheType.CAFFEINE_WTLFU);
constants.put(StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_MAXSIZE, 10000);
BOOTSTRAP_CONSTANTS = Collections.unmodifiableMap(constants);
}

protected static final Logger LOG = LogManager.getLogger(DefaultConfiguration.class);

// Programmatic Action Configurations
Expand Down Expand Up @@ -369,14 +389,9 @@ protected Container createBootstrapContainer(List<ContainerProvider> providers)

builder.factory(ValueSubstitutor.class, EnvsValueSubstitutor.class, Scope.SINGLETON);

builder.constant(StrutsConstants.STRUTS_DEVMODE, "false");
builder.constant(StrutsConstants.STRUTS_OGNL_LOG_MISSING_PROPERTIES, "false");
builder.constant(StrutsConstants.STRUTS_OGNL_ENABLE_EVAL_EXPRESSION, "false");
builder.constant(StrutsConstants.STRUTS_OGNL_ENABLE_EXPRESSION_CACHE, "true");
builder.constant(StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, "false");
builder.constant(StrutsConstants.STRUTS_I18N_RELOAD, "false");

builder.constant(StrutsConstants.STRUTS_MATCHER_APPEND_NAMED_PARAMETERS, "true");
for (Map.Entry<String, Object> entry : BOOTSTRAP_CONSTANTS.entrySet()) {
builder.constant(entry.getKey(), String.valueOf(entry.getValue()));
}

return builder.create(true);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@
*/
package com.opensymphony.xwork2.config.impl;

import com.opensymphony.xwork2.config.*;
import com.opensymphony.xwork2.config.Configuration;
import com.opensymphony.xwork2.config.ConfigurationException;
import com.opensymphony.xwork2.config.ContainerProvider;
import com.opensymphony.xwork2.config.PackageProvider;
import com.opensymphony.xwork2.config.RuntimeConfiguration;
import com.opensymphony.xwork2.config.entities.PackageConfig;
import com.opensymphony.xwork2.config.entities.UnknownHandlerConfig;
import com.opensymphony.xwork2.config.providers.StrutsDefaultConfigurationProvider;
Expand All @@ -28,7 +32,11 @@
import com.opensymphony.xwork2.util.location.LocatableProperties;
import org.apache.struts2.StrutsConstants;

import java.util.*;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;


/**
Expand All @@ -51,9 +59,9 @@ public void selfRegister() {
builder.factory(Configuration.class, MockConfiguration.class, Scope.SINGLETON);
LocatableProperties props = new LocatableProperties();
new StrutsDefaultConfigurationProvider().register(builder, props);
builder.constant(StrutsConstants.STRUTS_DEVMODE, "false");
builder.constant(StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, "true");
builder.constant(StrutsConstants.STRUTS_OGNL_ENABLE_EXPRESSION_CACHE, "true");
for (Map.Entry<String, Object> entry : DefaultConfiguration.BOOTSTRAP_CONSTANTS.entrySet()) {
builder.constant(entry.getKey(), String.valueOf(entry.getValue()));
}
builder.constant(StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, "false");
container = builder.create(true);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.opensymphony.xwork2.config.Configuration;
import com.opensymphony.xwork2.config.ConfigurationException;
import com.opensymphony.xwork2.config.ConfigurationProvider;
import com.opensymphony.xwork2.config.impl.DefaultConfiguration;
import com.opensymphony.xwork2.conversion.ConversionAnnotationProcessor;
import com.opensymphony.xwork2.conversion.ConversionFileProcessor;
import com.opensymphony.xwork2.conversion.ConversionPropertiesProcessor;
Expand Down Expand Up @@ -251,15 +252,11 @@ public void register(ContainerBuilder builder, LocatableProperties props)
.factory(ExecutorProvider.class, StrutsExecutorProvider.class, Scope.SINGLETON)
;

props.setProperty(StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, Boolean.FALSE.toString());
props.setProperty(StrutsConstants.STRUTS_I18N_RELOAD, Boolean.FALSE.toString());
props.setProperty(StrutsConstants.STRUTS_DEVMODE, Boolean.FALSE.toString());
props.setProperty(StrutsConstants.STRUTS_OGNL_LOG_MISSING_PROPERTIES, Boolean.FALSE.toString());
props.setProperty(StrutsConstants.STRUTS_OGNL_ENABLE_EXPRESSION_CACHE, Boolean.TRUE.toString());
props.setProperty(StrutsConstants.STRUTS_OGNL_ENABLE_EVAL_EXPRESSION, Boolean.FALSE.toString());
props.setProperty(StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, Boolean.FALSE.toString());
for (Map.Entry<String, Object> entry : DefaultConfiguration.BOOTSTRAP_CONSTANTS.entrySet()) {
props.setProperty(entry.getKey(), String.valueOf(entry.getValue()));
}

props.setProperty(StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, Boolean.TRUE.toString());
props.setProperty(StrutsConstants.STRUTS_MATCHER_APPEND_NAMED_PARAMETERS, Boolean.TRUE.toString());
props.setProperty(StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, Boolean.FALSE.toString());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.opensymphony.xwork2.ognl;

import com.opensymphony.xwork2.inject.Inject;
import org.apache.commons.lang3.EnumUtils;
import org.apache.struts2.StrutsConstants;

/**
Expand All @@ -29,16 +30,16 @@
public class DefaultOgnlBeanInfoCacheFactory<Key, Value> extends DefaultOgnlCacheFactory<Key, Value>
implements BeanInfoCacheFactory<Key, Value> {

@Override
@Inject(value = StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_MAXSIZE, required = false)
protected void setCacheMaxSize(String maxSize) {
super.setCacheMaxSize(maxSize);
/**
* @deprecated since 6.4.0, use {@link #DefaultOgnlBeanInfoCacheFactory(String, String)}
*/
@Deprecated
public DefaultOgnlBeanInfoCacheFactory() {
}

@Override
@Inject(value = StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_LRU_MODE, required = false)
protected void setUseLRUCache(String useLRUMode) {
super.setUseLRUCache(useLRUMode);
@Inject
public DefaultOgnlBeanInfoCacheFactory(@Inject(value = StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_MAXSIZE) String cacheMaxSize,
@Inject(value = StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_TYPE) String defaultCacheType) {
super(Integer.parseInt(cacheMaxSize), EnumUtils.getEnumIgnoreCase(CacheType.class, defaultCacheType));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,52 +15,86 @@
*/
package com.opensymphony.xwork2.ognl;

import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import org.apache.commons.lang3.BooleanUtils;

/**
* Default OGNL Cache factory implementation.
* <p>Default OGNL Cache factory implementation.</p>
*
* Currently used for Expression cache and BeanInfo cache creation.
* <p>Currently used for Expression cache and BeanInfo cache creation.</p>
*
* @param <Key> The type for the cache key entries
* @param <Key> The type for the cache key entries
* @param <Value> The type for the cache value entries
*/
public class DefaultOgnlCacheFactory<Key, Value> implements OgnlCacheFactory<Key, Value> {

private final AtomicBoolean useLRUCache = new AtomicBoolean(false);
private final AtomicInteger cacheMaxSize = new AtomicInteger(25000);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this factory class thread-safe seemed unnecessary

private static final int DEFAULT_INIT_CAPACITY = 16;
private static final float DEFAULT_LOAD_FACTOR = 0.75f;

private CacheType defaultCacheType;
private int cacheMaxSize;

/**
* @deprecated since 6.4.0, use {@link #DefaultOgnlCacheFactory(int, CacheType)}
*/
@Deprecated
public DefaultOgnlCacheFactory() {
this(10000, CacheType.CAFFEINE_WTLFU);
}

public DefaultOgnlCacheFactory(int cacheMaxSize, CacheType defaultCacheType) {
this.cacheMaxSize = cacheMaxSize;
this.defaultCacheType = defaultCacheType;
}

@Override
public OgnlCache<Key, Value> buildOgnlCache() {
return buildOgnlCache(getCacheMaxSize(), 16, 0.75f, getUseLRUCache());
return buildOgnlCache(getCacheMaxSize(), DEFAULT_INIT_CAPACITY, DEFAULT_LOAD_FACTOR, defaultCacheType);
}

@Override
public OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, boolean lruCache) {
if (lruCache) {
return new OgnlLRUCache<>(evictionLimit, initialCapacity, loadFactor);
} else {
return new OgnlDefaultCache<>(evictionLimit, initialCapacity, loadFactor);
public OgnlCache<Key, Value> buildOgnlCache(int evictionLimit,
int initialCapacity,
float loadFactor,
CacheType cacheType) {
switch (cacheType) {
case CONCURRENT_BASIC:
return new OgnlDefaultCache<>(evictionLimit, initialCapacity, loadFactor);
case SYNC_LINKED_LRU:
return new OgnlLRUCache<>(evictionLimit, initialCapacity, loadFactor);
case CAFFEINE_WTLFU:
return new OgnlCaffeineCache<>(evictionLimit, initialCapacity);
default:
throw new IllegalArgumentException("Unknown cache type: " + cacheType);
}
}

@Override
public int getCacheMaxSize() {
return cacheMaxSize.get();
return cacheMaxSize;
}

/**
* @deprecated since 6.4.0
*/
@Deprecated
protected void setCacheMaxSize(String maxSize) {
cacheMaxSize.set(Integer.parseInt(maxSize));
cacheMaxSize = Integer.parseInt(maxSize);
}

@Override
public boolean getUseLRUCache() {
return useLRUCache.get();
public CacheType getDefaultCacheType() {
return defaultCacheType;
}

/**
* No effect when {@code useLRUMode} is {@code false}
*
* @deprecated since 6.4.0
*/
@Deprecated
protected void setUseLRUCache(String useLRUMode) {
useLRUCache.set(BooleanUtils.toBoolean(useLRUMode));
if (BooleanUtils.toBoolean(useLRUMode)) {
defaultCacheType = CacheType.SYNC_LINKED_LRU;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,29 +16,30 @@
package com.opensymphony.xwork2.ognl;

import com.opensymphony.xwork2.inject.Inject;
import org.apache.commons.lang3.EnumUtils;
import org.apache.struts2.StrutsConstants;

/**
* Default OGNL Expression Cache factory implementation.
*
* <p>
* Currently used for Expression cache creation.
*
* @param <Key> The type for the cache key entries
* @param <Key> The type for the cache key entries
* @param <Value> The type for the cache value entries
*/
public class DefaultOgnlExpressionCacheFactory<Key, Value> extends DefaultOgnlCacheFactory<Key, Value>
implements ExpressionCacheFactory<Key, Value> {
implements ExpressionCacheFactory<Key, Value> {

@Override
@Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE, required = false)
protected void setCacheMaxSize(String maxSize) {
super.setCacheMaxSize(maxSize);
/**
* @deprecated since 6.4.0, use {@link #DefaultOgnlExpressionCacheFactory(String, String)}
*/
@Deprecated
public DefaultOgnlExpressionCacheFactory() {
}

@Override
@Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_LRU_MODE, required = false)
protected void setUseLRUCache(String useLRUMode) {
super.setUseLRUCache(useLRUMode);
@Inject
public DefaultOgnlExpressionCacheFactory(@Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE) String cacheMaxSize,
@Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_TYPE) String defaultCacheType) {
super(Integer.parseInt(cacheMaxSize), EnumUtils.getEnumIgnoreCase(CacheType.class, defaultCacheType));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,52 @@
* Used by {@link com.opensymphony.xwork2.ognl.OgnlUtil} to create appropriate OGNL
* caches based on configuration.
*
* @param <Key> The type for the cache key entries
* @param <Key> The type for the cache key entries
* @param <Value> The type for the cache value entries
*/
interface OgnlCacheFactory<Key, Value> {
public interface OgnlCacheFactory<Key, Value> {
OgnlCache<Key, Value> buildOgnlCache();
OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, boolean lruCache);

/**
* Note that if {@code lruCache} is {@code false}, the cache type could still be LRU if the default cache type is
* configured as such.
* @deprecated since 6.4.0, use {@link #buildOgnlCache(int, int, float, CacheType)}
*/
@Deprecated
default OgnlCache<Key, Value> buildOgnlCache(int evictionLimit,
int initialCapacity,
float loadFactor,
boolean lruCache) {
return buildOgnlCache(evictionLimit,
initialCapacity,
loadFactor,
lruCache ? CacheType.SYNC_LINKED_LRU : getDefaultCacheType());
}

/**
* @param evictionLimit maximum capacity of the cache where applicable for cache type chosen
* @param initialCapacity initial capacity of the cache where applicable for cache type chosen
* @param loadFactor load factor of the cache where applicable for cache type chosen
* @param cacheType type of cache to build
* @return a new cache instance
*/
OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, CacheType cacheType);

int getCacheMaxSize();
boolean getUseLRUCache();

/**
* @deprecated since 6.4.0
*/
@Deprecated
default boolean getUseLRUCache() {
return CacheType.SYNC_LINKED_LRU.equals(getDefaultCacheType());
}

CacheType getDefaultCacheType();

enum CacheType {
CONCURRENT_BASIC,
SYNC_LINKED_LRU,
CAFFEINE_WTLFU
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these different implementations of cache layer? Shouldn't this go through the extension point?

Copy link
Member Author

@kusalk kusalk Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering the same thing - I was only maintaining the design introduced in #528.

I simply replaced struts.ognl.expressionCacheLRUMode=true|false with struts.ognl.expressionCacheType=basic|lru|wtlfu

Let me look into refactoring this, it will likely be a breaking change though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah given the breaking nature, I'm more inclined to take on this refactor as part of 7.0, all good for me to create a follow-up card for it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! That would make more sense and then we can remove all the properties and leave that to a given implementation. Please create the ticket :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
}
Loading