Bug 23732 – Rework CachedResource::overheadSize accounting

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

Created attachment 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(-)

[Darin Adler]

Comment on attachment 27318 [details]
Rework CachedResource overhead accounting, v1

> -    // 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.

[Geoffrey Garen]

Is it really helpful to make a guess slightly more accurate? Is there a known case of memory growth that this patch would fix?

[Dimitri Glazkov (Google)]

(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.

[Dimitri Glazkov (Google)]

Created attachment 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(-)

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

Created attachment 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(-)

[Dimitri Glazkov (Google)]

Comment on attachment 27328 [details]
Rework CachedResource overhead accounting, v2.1

Whoops. Funny math in the latest patch. Fix coming up.

[Dimitri Glazkov (Google)]

Created attachment 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(-)

[Dimitri Glazkov (Google)]

Comment on attachment 27332 [details]
Rework CachedResource overhead accounting, v2.2

Now it's ready, I think.

[Dimitri Glazkov (Google)]

Ping?

[Geoffrey Garen]

Comment on attachment 27332 [details]
Rework CachedResource overhead accounting, v2.2

r=me

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/41092.