Potential expression cache enhancement for 2.6 series

[JCgH4164838Gh792C124B5]

Update:

• Add support for an optional basic LRU cache for OGNL expressions and OGNL BeanInfo.
• Add support for cache limits applying to both normal and LRU caches. For a normal cache the entire cache will flush when the limit is reached.
• Add flags to allow switching between normal and LRU caches, and setting the maximum sizes.

[JCgH4164838Gh792C124B5]

Hello Apache Struts Team.

This PR contains a potential enhancement for the expression and BeanInfo cache implementation in the 2.6 branch. Others may have other ideas, or more efficient implementations to consider, but it may still provide a starting point for an implementation.

If the idea seems reasonable, then a new JIRA ticket can be opened and associated with the proposed change. If not, then the PR can be closed.

[lukaszlenart]

Clever idea, yet I would do it a bit different. It would be better to extract existing cache layer into dedicated classes and also extract the common interfaces. Then add new LRU implementation to the stack. And then depending on the flags create a proper instance.

Such approach will eliminate in-method logic to select a proper reference, instead this:

final Map<String, Object> expressionsCacheRef = (useLRUExpressionCache.get() ? expressionsCacheLRU.backingMapReference() : expressionsCache);
tree = expressionsCacheRef.get(expression);

you will just use already defined reference

tree = expressionsCacheRef.get(expression);

wdyt?

[coveralls]

Coverage increased (+0.07%) to 50.61% when pulling b72200a on JCgH4164838Gh792C124B5:localS2_26_OgnlUtilOptionalCache1 into bea1c6f on apache:master.

[JCgH4164838Gh792C124B5]

Hi @lukaszlenart .

Thanks for the feedback and suggestions.

Moving the existing cache layer into dedicated interfaces/classes is probably cleaner in the long run, as you pointed out. I can attempt to do that in a revision to the PR, it just might take me a little while.

Because the cache instance(s) are final and part of the intializer block, the instances get set before the configuration flags can be applied (when the framework injects the flag values a little later). A dedicated cache implementation could probably help address that (or at least hide some of the details), in a cleaner way than what I have done so far.

I will try a refactor and see how it works out.

[lukaszlenart]

You can inject them via constructor if you want to or provide a factory which will be injected into OgnlUtil and this factory will be used to create a proper instance of cache:

public OgnlUtil(
    @Inject(StrutsConstants.STRUTS_EXPRESSION_CACHE_FACTORY, required = false) ExpressionCacheFactory cacheFactory
) {
    if (cacheFactory == null) {
        // fallback to old mechanism
    } else {
       this.cache = cacheFactory.createCacheInstance();
    }
}

and the factory:

class ExpressionCacheFactory {

    @Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE, required = false)
    protected void setExpressionsCacheMaxSize(String maxSize) {
        expressionsCacheMaxSize.set(Integer.parseInt(maxSize));
        expressionsCacheLRU.setEvictionLimit(expressionsCacheMaxSize.get());
    }

    @Inject(value = StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_MAXSIZE, required = false)
    protected void setBeanInfoCacheMaxSize(String maxSize) {
        beanInfoCacheMaxSize.set(Integer.parseInt(maxSize));
        beanInfoCacheLRU.setEvictionLimit(beanInfoCacheMaxSize.get());
    }

    @Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_LRU_MODE, required = false)
    protected void setUseLRUExpressionCache(String useLRUMode) {
        useLRUExpressionCache.set(BooleanUtils.toBoolean(useLRUMode));
    }

    @Inject(value = StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_LRU_MODE, required = false)
    protected void setUseLRUBeanInfoCache(String useLRUMode) {
        useLRUBeanInfoCache.set(BooleanUtils.toBoolean(useLRUMode));
    }

    public CacheInstance createCacheInstance() {
          ....
     }

}

Then you just need to define a bean in struts-default.xml and done :)

Take your time and if you need my help let me know. Or if you want to, I can refactor your code once we merge this PR - working with DI in Struts can be frustrating on the beginning ;-)

[JCgH4164838Gh792C124B5]

Hi @lukaszlenart .

The PR has been updated after refactoring to something more along the lines of what you suggested, and then resolving a merge conflict. There are likely more improvements to be made, but it is probably closer to something workable now.

[lukaszlenart]

Brilliant 💯 👏
Do you plan updating the docs [1]?

[1] https://struts.apache.org/tag-developers/
(maybe we should move the OGNL section to the Core Developers Guide?)

[JCgH4164838Gh792C124B5]

Hi @lukaszlenart.

Thanks for the additional feedback.

I had not considered the documentation side of things in relation to the potential change. The existing documentation section for OGNL expression in the Tag Developer's Guide seems to be OK where it is, since most of the interaction and examples are via tags.

If this PR (or something similar) goes forward, then maybe a new section in the Core Developer's Guide that addresses configuration of the default OGNL expression and BeanInfo caches would make sense. I could try drafting something for that, in that case.

Do you (or anyone else) see any weaknesses, design issues, or any other concerns with the current refactored code ? If so, please advise, and I can attempt to re-work things as needed.

Should we create a JIRA for tracking for the potential change as well ?

[lukaszlenart]

What do you think about moving this code into OgnlExpressionCacheFactory#createDefaultCache()?

[JCgH4164838Gh792C124B5]

The latest commit introduces a zero-parameter build method, which is a little cleaner. Since the factory parameter could be null, there is still a need to check for that possibility somewhere.

A static generator method could be introduced, something like DefaultOgnlExpressionCacheFactory#createDefaultCache(expressionCacheFactoryInstance), but it would not seem to improve things much ?

Did I misinterpret what you were suggesting ?

[lukaszlenart]

Ah, ognlExpressionCacheFactory represents interface, I missed that. And I thought about wrapping new OgnlDefaultCache<>(25000, 16, 0.75f); into a factory method within the factory:

public OgnlCache<> buildDefaultCache() {
    return new OgnlDefaultCache<>(25000, 16, 0.75f);
}

[lukaszlenart]

Same here, OgnlExpressionCacheFactory#createDefaultCache()?

[JCgH4164838Gh792C124B5]

Please see the comment for the expression cache item above.

[lukaszlenart]

Should we create a JIRA for tracking for the potential change as well ?

Up to you, it allows users to monitor what has changed plus you can link the new documentation to the ticket as well.

[JCgH4164838Gh792C124B5]

Hi @lukaszlenart .
While attempting to put together some documentation on configuring the new caches, I found that I could not successfully register a custom implementation (only the default implementation ever got registered).
Going over the code for other factories, I noticed that there was processing in ConstantConfig and StrutsBeanSelectionProvider that is not present in this PR. Adding equivalent logic for the two new cache factories locally did not seem to work, either (when using a configuration patterned after the custom TextProviderFactory example). I attempted to debug the initialization, but cannot find what is missing.
The factory initialization processing is complex, so there is probably something I am not understanding in order to make things work as intended.
If you (or anyone reading this) can suggest where I should look next, or suggest another example of custom factory initialization that might make things clearer, it would be appreciated. 😄

[lukaszlenart]

Yeah... I'm planning to write a guideline how to use Struts DI mechanism, it can be hard to understand on the first glimpse ;-)

You can try to follow my changes in this PR - I introduced an injectable date formatter with two default implementations, but users can provide they own if needed.

So the whole trick is that you must define an extension point: struts.ognl.expressionCacheFactory (in my PR it was struts.date.formatter) and supporting interface/class that will be used together OgnlCacheFactory (DateFormatter in my PR). Now in StrutsBeanSelectionProvider you must register an alias which ties the extension point with the backing interface/class - in my case it was

alias(DateFormatter.class, StrutsConstants.STRUTS_DATE_FORMATTER, builder, props, Scope.SINGLETON);

(your PR is missing this step, sorry that I didn't spot it ealy :( )

Having that set, either you can make this a required dependency and provide at least one implementation of the interface/class or make it optional and handle nulls in your code.

In my case I made it required and provided two implementations: SimpleDateFormatAdapter named simpleDateFormatAdapter (as a bean name) and DateTimeFormatterAdapter named dateTimeFormatterAdapter - the names can be of your choice, they to not play any role yet.

Now you must register the beans in Struts DI:

<bean type="org.apache.struts2.components.date.DateFormatter" name="simpleDateFormatter" class="org.apache.struts2.components.date.SimpleDateFormatAdapter" scope="singleton"/>
<bean type="org.apache.struts2.components.date.DateFormatter" name="dateTimeFormatter" class="org.apache.struts2.components.date.DateTimeFormatterAdapter" scope="singleton"/>

^^ this ties implementation with the name.

And now it's time to tell Struts DI which implementation to use for the extension point:

struts.date.formatter=dateTimeFormatter

and done! :)

With such approach users can define they own implementations and used them with the extension point.

If something is still unclear please ask, I will try to prepare the guideline.

[lukaszlenart]

I've started preparing the guideline apache/struts-site#160

[JCgH4164838Gh792C124B5]

Hello @lukaszlenart . Thank you for the explanation concerning the DI mechanism flow, the reference example PR, and putting together the guideline in the site documentation. 😄

The PR has been updated, but still has some issues.

I did my best to try and get the extension points to work for customization, but the custom implementation failed to be used every time. 😞 No matter how I tweaked things (including adding elements that some other extension points seemed to also define), only the default implementation ended up initialized/used (either that or the configuration prevented the framework from initializing).

Debugging a sample app (I tried using a slightly modified showcase for convenience) it also seems that depending on what stage initialization is at, the DI configuration values (for cache size and mode) are sometimes available and sometimes not (almost as if order or timing related).

Given the issues encountered so far, I am concerned that this proposed PR implementation is somehow fundamentally broken (due to something being wrong with its DI implementation, or an ordering/timing limitation).

What do you (and others) think ?

[lukaszlenart]

Let me work a bit on you PR, maybe something obvious is missing (which happens to me each time when I need to work with Struts DI ;) )

[lukaszlenart]

PR fixing the problem is ready JCgH4164838Gh792C124B5#1

[lukaszlenart]

All set, LGTM 👍 thanks @JCgH4164838Gh792C124B5 👏