Add caching to CombinedResourceHandler #100

Closed
stephanrauh opened this Issue Feb 15, 2015 · 12 comments

Projects

None yet

3 participants

@stephanrauh
Contributor

I've implemented a prototype of a caching CombinedResourceHandler (see https://github.com/stephanrauh/AngularFaces/tree/master/AngularFaces_2.0/AngularFaces.net/src/main/java/org/omnifaces/cachingresourcehandler). In my case, it reduced the network delays significantly, especially for numerous and large Javascript files (10 times faster).

I'd like to make my prototype production-ready and contribute it to OmniFaces. How do you like the idea?

@BalusC
Member
BalusC commented Feb 16, 2015

Nice. Would be more nice if it reused the codebase and was configurable via a boolean context param which defaults to false.

Don't forget to document a warning that this has a significant memory cost.

@arjantijms
Member

Nice indeed. And would indeed be just that bit nicer if it used the existing Cache interface of OmniFaces if possible.

That way, users can set a maximum to the number of cached entries and/or connect it to a cache that can do eviction based on time not used, size (number of bytes used) etc.

@stephanrauh
Contributor

Hi Bauke, hi Arjan,

thanks for your feedback! Yeah, my prototype was a sunday afternoon hack :). I didn't want to invest more time until I knew for sure I hadn't missed a caching feature that's already there.

if it used the existing Cache interface

Thanks for the hint - I didn't know this interface yet.

Don't forget to document a warning that this has a significant memory cost.

Where do you want me to document it? In the showcase application?

Regards,
Stephan

@stephanrauh
Contributor

Done.

  • The cache is always put in the application scope. Session scope is more or less pointless because of browser caching.
  • The context parameter org.omnifaces.COMBINED_RESOURCE_ACTIVATE_RESOURCE_CACHING activates caching.
  • The optional context parameters org.omnifaces.CACHE_SETTING_APPLICATION_MAX_CAPACITY and org.omnifaces.CACHE_SETTING_APPLICATION_TTL now are also responsible for the maximum resource cache capacity.
  • Cache interface is used. Please verify if I got the second parameter of putAttribute/getAttribute right. I looks wrong to me, but I don't know what it's supposed to be:
scopedCache.getAttribute(key, key)
scopedCache.putAttribute(key, key, _combinedResource,getTimeToLiveOfCacheEntries())

ToDos: documentation and refactoring the code (it looks a bit ugly because I didn't want to modify the original implementation more than necessary).

  <!-- activate resource caching -->
  <context-param>
    <param-name>org.omnifaces.COMBINED_RESOURCE_ACTIVATE_RESOURCE_CACHING</param-name>
    <param-value>
      true
    </param-value>
  </context-param>
  <!-- cache ten combined resources at most -->
  <context-param>
    <param-name>org.omnifaces.CACHE_SETTING_APPLICATION_MAX_CAPACITY</param-name>
    <param-value>10</param-value>
  </context-param>

  <!-- set the maximum time-to-live of cache entries to one day -->
  <context-param>
    <param-name>org.omnifaces.CACHE_SETTING_APPLICATION_TTL</param-name>
    <param-value>86400</param-value>
  </context-param>

Here's an example of the effect. The first two requests are served directly, the other two requests are served from the cache:

image

@arjantijms
Member

At a quick glance it looks quite good, thanks!

Please verify if I got the second parameter of putAttribute/getAttribute right. I looks wrong to me, but I don't know what it's supposed to be:

It's not entirely right indeed. putAttribute/getAttribute are for multi-maps (a 2 level tree map). In other words, when you have multiple separate entries that need to be get and put individually, but still share a common TTL and expiration etc.

When all you have is a single value, you're probably better off using the put(key, value, ttl) and get(key) methods.

@stephanrauh
Contributor

Well, yes - if there were such a thing :). To follow your advice, I added getObject() and putObject() because get() and put() work with Strings. Probably it's a better idea to change the signature of get() and put(), but I'd rather leave that to you - you know the consequences better than I do.

The new version should be visible in the pull request now.

@arjantijms
Member

You're right. The interface originally wasn't really intended for broad general use, but as things go overtime grew to become exactly that. The original get/put are String based since the cache only cached rendered markup.

I'll mediate a little over changing the signature of get & put. One consequence is of course that people who have already implemented it themselves may see their code breaking, but I think OmniFaces is still small enough to get away with that.

@BalusC
Member
BalusC commented Feb 17, 2015

You could parameterize it. It should only end up in "raw type" warnings on existing implementations.

While at it, I think moving all classes of org.omnifaces.component.output.cache into another package outside org.omnifaces.component wouldn't be a bad idea either. Namely, no one class has a dependency on the UIComponent (sub)class while the package structure suggests it being that unreusable.

@stephanrauh
Contributor

I ran a Google search. I found only one blog dealing with the Cache class - and it was about deleting entries. So I suppose hardly anybody knows the API, and you can get away with anything.

Moving the package and introducing generics both break existing implementations relying on Cache (the latter because String get() becomes Object get(), which requires a type cast to String in existing implementations). That said, I support parameterizing the Cache class. As I said, hardly anyone uses it.

@stephanrauh
Contributor

I added the documentation you requested. It took me a while to find it - extracting the showcase from Javadoc is clever :). Along the way I found and fixed a minor bug (incomplete migration from Cache.getAttribute() to Cache.getObject()).

@BalusC BalusC added a commit that referenced this issue Mar 2, 2015
@BalusC BalusC #100 #103 Moved and refactored caching from CombinedResourceInputStream
into CombinedResource, hereby restoring original
CombinedResourceInputStream. Replaced two configuration context
parameters by one. Turned leading spaces back to tabs. Moved missing
parts of Cache javadoc back from o:cache vdldoc. Took the opportunity to
make package-private CombinedResourceXxx classes public. Improved
javadoc.
f2c9389
@BalusC
Member
BalusC commented Mar 2, 2015

Thank you very much for the initial commit. I made following improvements on the implementation:

  • Moved caching logic from CombinedResourceInputStream to CombinedResource, hereby simplifying the here and there duplicated code logic. With this, the original CombinedResourceInputStream code is entirely restored.
  • Merged two context params into one representing just TTL. If JSF project stage is not set to development, then any value greater than 0 will enable the caching.
  • Initialize the cache TTL only once during CombinedResourceHandler construction instead of on every request. And, throw IAE when context param is wrong instead of silently ignoring it.
  • Clarified javadoc here and there. And, replaced javadoc references related to Cache configuration by javadoc link to Cache class.
  • Replaced leading spaces by tabs.
  • Moved credit from comments to @author of CombinedResourceHandler and CombinedResource.
@BalusC BalusC closed this Mar 2, 2015
@stephanrauh
Contributor

Thanks! Your changes are amazing: the code looks a lot cleaner now. Carry on the good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment