Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.2 Final
    • Fix Version/s: 4.3 Beta1
    • Component/s: HttpCache
    • Labels:
      None

      Description

      We're using the CachingHttpClient and are seeing a spike in CPU usage when it is enabled. We've profiled our application and see that most of the time is being spent parsing dates. Specifically, it is trying to get the age of a cache entry on a cache hit by parsing the "Date" header on the HttpCacheEntry. I had a couple questions:

      1) Why can't this use the responseDate value that lives on HttpCacheEntry? (This would avoid the overhead of parsing)
      2) If it needs to parse, is it possible to remember the result on the HttpCacheEntry so it doesn't need to be parsed every time?

      We are using version 4.2

      Here is the full backtrace we are seeing:

      org.apache.http.impl.cookie.DateUtils.parseDate(String)
      org.apache.http.impl.client.cache.CacheValidityPolicy.getDateValue(HttpCacheEntry)
      org.apache.http.impl.client.cache.CacheValidityPolicy.getApparentAgeSecs(HttpCacheEntry)
      org.apache.http.impl.client.cache.CacheValidityPolicy.getCorrectedReceivedAgeSecs(HttpCacheEntry)
      org.apache.http.impl.client.cache.CacheValidityPolicy.getCorrectedInitialAgeSecs(HttpCacheEntry)
      org.apache.http.impl.client.cache.CacheValidityPolicy.getCurrentAgeSecs(HttpCacheEntry, Date)

      There are a couple callers to "getCorrectedAgeSecs":

      CacheValidityPolicy.isResponseFresh(HttpCacheEntry, Date)
      CachedResponseSuitabilityChecker.isFreshEnough(HttpCacheEntry, HttpRequest, Date)
      CachedResponseSuitabilityChecker.canCachedResponseBeUsed(HttpHost, HttpRequest, HttpCacheEntry, Date)
      CachingHttpClient.handleCacheHit(HttpHost, HttpRequest, HttpContext, HttpCacheEntry)

      CachedHttpResponseGenerator.generateResponse(HttpCacheEntry)
      CachingHttpClient.generateCachedResponse(HttpRequest, HttpContext, HttpCacheEntry, Date)
      CachingHttpClient.handleCacheHit(HttpHost, HttpRequest, HttpContext, HttpCacheEntry)

      Looking at the code, it looks like this section from CachingHttpClient.handleCacheHit will result in parsing the date twice (apologies if I'm misreading this)

      if (suitabilityChecker.canCachedResponseBeUsed(target, request, entry, now))

      { return generateCachedResponse(request, context, entry, now); }

      Both the call to "canCachedResponseBeUsed" and the call to "generatedCachedResponse" will ultimately call "getCurrentAgeSecs" and parse the Date header.

        Activity

        Hide
        Oleg Kalnichevski added a comment -

        No, not really (we never got around to formalizing our versioning policies) but is consistent with the requirements of Semantic Versioning [1]

        Oleg

        [1] http://semver.org/

        Show
        Oleg Kalnichevski added a comment - No, not really (we never got around to formalizing our versioning policies) but is consistent with the requirements of Semantic Versioning [1] Oleg [1] http://semver.org/
        Hide
        Sebb added a comment -

        "Ideally such changes should not take place on the stable branch"

        Is this documented somewhere?

        Show
        Sebb added a comment - "Ideally such changes should not take place on the stable branch" Is this documented somewhere?
        Hide
        Oleg Kalnichevski added a comment -

        Sam,
        Your patch adds a new public method to a public class and deprecates an existing one. Ideally such changes should not take place on the stable branch unless when fixing some critical issue.

        Patch committed to SVN trunk.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Sam, Your patch adds a new public method to a public class and deprecates an existing one. Ideally such changes should not take place on the stable branch unless when fixing some critical issue. Patch committed to SVN trunk. Oleg
        Hide
        Sam Perman added a comment -

        I'm pretty sure my patch is compatible with the 4.2 line... any chance this can move into a 4.2 patch instead of waiting for 4.3?

        Show
        Sam Perman added a comment - I'm pretty sure my patch is compatible with the 4.2 line... any chance this can move into a 4.2 patch instead of waiting for 4.3?
        Hide
        Sam Perman added a comment -

        Here is a version of the patch that retains that protected method (and deprecates it.)

        Show
        Sam Perman added a comment - Here is a version of the patch that retains that protected method (and deprecates it.)
        Hide
        Oleg Kalnichevski added a comment -

        Ideally we should preserve full binary compatibility with the 4.2.x line, so the protected method in question should be deprecated, not removed.

        You can use 'mvn:clirr check' command to make sure your private copy is still fully compatible with the 4.2 GA release.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Ideally we should preserve full binary compatibility with the 4.2.x line, so the protected method in question should be deprecated, not removed. You can use 'mvn:clirr check' command to make sure your private copy is still fully compatible with the 4.2 GA release. Oleg
        Hide
        Sam Perman added a comment -

        Just a note on that patch: I deleted the protected method "CacheValidityPolicy.getDateValue". I'm not sure what your policy is on doing things like that, so if it needs to be left in, it would look like this:

        protected Date getDateValue(final HttpCacheEntry entry)

        { return entry.getDateHeaderValue(); }
        Show
        Sam Perman added a comment - Just a note on that patch: I deleted the protected method "CacheValidityPolicy.getDateValue". I'm not sure what your policy is on doing things like that, so if it needs to be left in, it would look like this: protected Date getDateValue(final HttpCacheEntry entry) { return entry.getDateHeaderValue(); }
        Hide
        Sam Perman added a comment -

        Here is a proposed fix for this issue. It will parse the Date header with the HttpCacheEntry is created rather than each time it is accessed.

        Show
        Sam Perman added a comment - Here is a proposed fix for this issue. It will parse the Date header with the HttpCacheEntry is created rather than each time it is accessed.
        Hide
        Sam Perman added a comment -

        As mentioned on the mailing list, using the responseDate on the HttpCacheEntry per RFC2616 but maybe the result of the parsing can be remembered.

        Show
        Sam Perman added a comment - As mentioned on the mailing list, using the responseDate on the HttpCacheEntry per RFC2616 but maybe the result of the parsing can be remembered.

          People

          • Assignee:
            Unassigned
            Reporter:
            Sam Perman
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development