Bug 23732 - Rework CachedResource::overheadSize accounting
: Rework CachedResource::overheadSize accounting
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-02-04 09:52 PST by
Modified: 2009-02-19 16:33 PST (History)


Attachments
Rework CachedResource overhead accounting, v1 (5.01 KB, patch)
2009-02-04 10:13 PST, Dimitri Glazkov (Google)
darin: review-
Review Patch | Details | Formatted Diff | Diff
Rework CachedResource overhead accounting, v2 (5.00 KB, patch)
2009-02-04 14:30 PST, Dimitri Glazkov (Google)
no flags Review Patch | Details | Formatted Diff | Diff
Rework CachedResource overhead accounting, v2.1 (5.43 KB, patch)
2009-02-04 14:33 PST, Dimitri Glazkov (Google)
no flags Review Patch | Details | Formatted Diff | Diff
Rework CachedResource overhead accounting, v2.2 (5.53 KB, patch)
2009-02-04 15:34 PST, Dimitri Glazkov (Google)
ggaren: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-02-04 09:52:02 PST
This patch is a slight rehash of bug 22747 with the intent to make the overhead size accounting a bit (just  a tiny little bit) more accurate and allow platforms implement their own accounting.
------- Comment #1 From 2009-02-04 10:13:37 PST -------
Created an attachment (id=27318) [details]
Rework CachedResource overhead accounting, v1

 WebCore/ChangeLog                               |   18 ++++++++++++++++++
 WebCore/loader/CachedResource.cpp               |   17 ++++-------------
 WebCore/platform/network/ResourceResponseBase.h |    2 ++
 WebCore/platform/network/cf/ResourceResponse.h  |   12 ++++++++++++
 WebCore/platform/network/mac/ResourceResponse.h |   12 ++++++++++++
 5 files changed, 48 insertions(+), 13 deletions(-)
------- Comment #2 From 2009-02-04 10:28:54 PST -------
(From update of attachment 27318 [details])
> -    // FIXME: Find some programmatic lighweight way to calculate response size, and size of the different CachedResource classes.  
> -    // This is a rough estimate of resource overhead based on stats collected from the stress test.
> -    return sizeof(CachedResource) + 3648;
> -    
> -    /*  sizeof(CachedResource) + 
> -        192 +                        // average size of m_url.
> -        384 +                        // average size of m_clients hash map.
> -        1280 * 2 +                   // average size of ResourceResponse.  Doubled to account for the WebCore copy and the CF copy. 
> -                                     // Mostly due to the size of the hash maps, the Header Map strings and the URL.
> -        256 * 2                      // Overhead from ResourceRequest, doubled to account for WebCore copy and CF copy. 
> -                                     // Mostly due to the URL and Header Map.
> -    */
> +    return sizeof(CachedResource) +
> +        m_response.size() +
> +        m_url.length() * 2 +
> +        384; // average size of m_clients hash map

If we're not going to have a comment on each line, then I think we should write the expression on one line in a more normal fashion.

I don't think size() is a good name for the "amount of memory used" function in ResourceResponse. We use that name for simpler things such as the size of a vector, and I'd prefer to use a name that makes more clear that this is supposed to be a deep size that includes malloc overhead and other things like that.

I don't think that the URL length * 2 is a great way to account for the size of the URL, and using it is a change in behavior from the old version. It's true that the characters are 2 bytes each (that should be sizeof(UChar) for clarity, not 2), but just computing the length ignores malloc overhead and other overhead of the various memory blocks used String (or is it KURL?) class.

> +    unsigned size() const { return 0; }

Is 0 a good default? Wouldn't some arbitrary non-zero number be better for ports that don't go out of their way to optimize this?

I'm going to say review- because of the URL length issue above.
------- Comment #3 From 2009-02-04 11:57:02 PST -------
Is it really helpful to make a guess slightly more accurate? Is there a known case of memory growth that this patch would fix?
------- Comment #4 From 2009-02-04 12:29:51 PST -------
(In reply to comment #3)
> Is it really helpful to make a guess slightly more accurate? Is there a known
> case of memory growth that this patch would fix?
> 

The accuracy part of this patch is technically non-goal. I just viewed this as a low-hanging fruit and can totally remove this.

The divergence in ways the accounting is done, however, is logical and useful. Since ResourceResponse implementation is different from platform to platform, there should be a way to supply different sizes for it. For instance, in Chromium this was causing an undue pressure on cache, effectively reducing its size.
------- Comment #5 From 2009-02-04 14:30:21 PST -------
Created an attachment (id=27326) [details]
Rework CachedResource overhead accounting, v2

 WebCore/ChangeLog                               |   18 ++++++++++++++++++
 WebCore/loader/CachedResource.cpp               |   17 ++++-------------
 WebCore/platform/network/ResourceResponseBase.h |    2 ++
 WebCore/platform/network/cf/ResourceResponse.h  |   12 ++++++++++++
 WebCore/platform/network/mac/ResourceResponse.h |   12 ++++++++++++
 5 files changed, 48 insertions(+), 13 deletions(-)
------- Comment #6 From 2009-02-04 14:33:14 PST -------
I updated the patch according to your suggestions:

* renamed size() to memoryUsage()
* reverted back to using estimated averages for KURL overhead
* initialized ResourceResponse::memoryUsage() with the average size, lifted from what's in trunk
------- Comment #7 From 2009-02-04 14:33:57 PST -------
Created an attachment (id=27328) [details]
Rework CachedResource overhead accounting, v2.1

 WebCore/ChangeLog                               |   18 ++++++++++++++++++
 WebCore/loader/CachedResource.cpp               |   16 ++++------------
 WebCore/platform/network/ResourceResponseBase.h |    7 +++++++
 WebCore/platform/network/cf/ResourceResponse.h  |   12 ++++++++++++
 WebCore/platform/network/mac/ResourceResponse.h |   12 ++++++++++++
 5 files changed, 53 insertions(+), 12 deletions(-)
------- Comment #8 From 2009-02-04 15:27:43 PST -------
(From update of attachment 27328 [details])
Whoops. Funny math in the latest patch. Fix coming up.
------- Comment #9 From 2009-02-04 15:34:48 PST -------
Created an attachment (id=27332) [details]
Rework CachedResource overhead accounting, v2.2

 WebCore/ChangeLog                               |   18 ++++++++++++++++++
 WebCore/loader/CachedResource.cpp               |   16 ++++------------
 WebCore/platform/network/ResourceResponseBase.h |    7 +++++++
 WebCore/platform/network/cf/ResourceResponse.h  |   12 ++++++++++++
 WebCore/platform/network/mac/ResourceResponse.h |   12 ++++++++++++
 5 files changed, 53 insertions(+), 12 deletions(-)
------- Comment #10 From 2009-02-04 15:35:44 PST -------
(From update of attachment 27332 [details])
Now it's ready, I think.
------- Comment #11 From 2009-02-11 16:27:11 PST -------
Ping?
------- Comment #12 From 2009-02-19 14:43:17 PST -------
(From update of attachment 27332 [details])
r=me
------- Comment #13 From 2009-02-19 16:33:53 PST -------
Landed as http://trac.webkit.org/changeset/41092.