Bug 57752 - WebResources Cache 'lookupCount' counts non-lookups
Summary: WebResources Cache 'lookupCount' counts non-lookups
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.0.20
Hardware: PC All
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-25 00:29 UTC by Adam Mlodzinski
Modified: 2015-03-26 21:51 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Mlodzinski 2015-03-25 00:29:43 UTC
The method org.apache.catalina.webresources.Cache.getResource(String, boolean) is used to retrieve a resource given a particular path.  Some resources are cached by the Cache class, while others bypass the cache.  A request for those resources which bypass the cache are forwarded somewhere else to find the resource (the 'root' object).
The problem is that every request for a resource, whether for a resource that is potentially in the cache, or for a resource which always bypasses the cache, is counted as a resource lookup, while the hit count is incremented only if the resource was found in the cache.  This leads to a highly disproportionate ratio of lookups to hits from the cache.

There are 2 ways to fix this:

1)  If a resource request bypasses the cache, do not increment the lookup count.

OR

2) If a resource request which bypasses the cache was found by the alternate source, then increment the hit count.

I think the 2nd option would lead to values higher than they ought to be.  The first option ought to provide more accurate representation of the usage of the cache.


For the fist option, the code fix is trivial:
{code}
*** Cache.java  2015-02-15 13:16:26.000000000 -0500
--- Cache.java.fixed    2015-03-24 19:08:52.630609500 -0400
***************
*** 59,70 ****

      protected WebResource getResource(String path, boolean useClassLoaderResources) {

-         lookupCount.incrementAndGet();
-
          if (noCache(path)) {
              return root.getResourceInternal(path, useClassLoaderResources);
          }

          CachedResource cacheEntry = resourceCache.get(path);

          if (cacheEntry != null && !cacheEntry.validateResource(useClassLoaderResources)) {
--- 59,70 ----

      protected WebResource getResource(String path, boolean useClassLoaderResources) {

          if (noCache(path)) {
              return root.getResourceInternal(path, useClassLoaderResources);
          }

+         lookupCount.incrementAndGet();
+
          CachedResource cacheEntry = resourceCache.get(path);

          if (cacheEntry != null && !cacheEntry.validateResource(useClassLoaderResources)) {
{code}
Comment 1 Mark Thomas 2015-03-26 21:51:20 UTC
Thanks for the report, the spot-on analysis and the patch. I agree the first option is beter.

Your patch has been applied to trunk and to 8.0.x for 8.0.22 onwards.

Thanks again.