Bug 57488 – Cached Resource Overhead Space Usage and Accounting Inaccurate

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Saboff]

The current CachedResource::overheadSize() on Mac returns 4312.  The ResourceResponse component of this is 3072.  Tests show that the ResourceResponse should actually be greater than 5100.

The suggested resolution is two fold, reduce the amount of memory ResourceResponse uses and then adjust the value returned by ResourceResponse::memoryUsage() accordingly.

[Michael Saboff]

Created attachment 87645 [details]
Patch to reduce ResourceResponse memory usage and bring overhead size inline with reality.

[WebKit Review Bot]

Attachment 87645 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:108:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Maciej Stachowiak]

Comment on attachment 87645 [details]
Patch to reduce ResourceResponse memory usage and bring overhead size inline with reality.

View in context: https://bugs.webkit.org/attachment.cgi?id=87645&action=review

r- because I think at least a few of my comments indicate real bugs.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:560
> +void ResourceResponseBase::lazyInit(bool baseInitOnly) const

Instead of a boolean, we usually prefer to use a named enum for this sort of thing, so it's clear what the parameter means at the call sites. I just read through a bunch of newly modified call sites and it was not at all clear what lazyInit(true) means.

> Source/WebCore/platform/network/cf/ResourceResponse.h:98
> +    void platformLazyInit(bool baseInitOnly = false);

It's a bad idea to have a default value for a parameter in the subclass, but not in the case class.

> Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:90
> +        // FIXME: We may need to do MIME type sniffing here (unless that is done in CFURLResponseGetMIMEType).

It is done in CFURLResponseGetMIMEType. You can remove the FIXME.

> Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:116
> +        for (int idx = 0; idx < numCommonHeaderFields; idx++) {
> +            CFStringRef value;
> +            if (CFDictionaryGetValueIfPresent(headers.get(), commonHeaderFields[i], &value))
> +                m_httpHeaderFields.set(commonHeaderFields[i], value);

We conventionally name for loop index variables just "i" rather than "idx". Some code below is subscripted by i, and I don't see an outer loop using i as the index, so I wonder if this is mistaken?

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:105
> +            for (int idx = 0; idx < numCommonHeaderFields; idx++) {
> +                if (NSString* headerValue = [headers objectForKey:commonHeaderFields[idx]])
> +                    m_httpHeaderFields.set([commonHeaderFields[idx] UTF8String], headerValue);

I suggest using i instead of idx for the index variable.

[Build Bot]

Attachment 87645 [details] did not build on win:
Build output: http://queues.webkit.org/results/8309089

[Michael Saboff]

Created attachment 87725 [details]
Patch with update in response to reviewer's comments

[Darin Adler]

Comment on attachment 87725 [details]
Patch with update in response to reviewer's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=87725&action=review

Sorry, didn’t finish my review, but I added a few comments.

> Source/WebCore/ChangeLog:17
> +        No new tests. (OOPS!)

You need to remove this line or replace it with an explanation of why there are no tests.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:258
> +    lazyInit(CommonFieldsOnly);
> +
> +    // Optimistic check first
> +    String value = m_httpHeaderFields.get(name);
> +    if (!value.isEmpty())        
> +        return value;
> +
> +    lazyInit(AllFields);

I think this “optimistic” check does not work well for fields that are common, but also commonly omitted from HTTP headers. We may want to explore a design where we know whether a field name is common or not rather than assuming anything not found is not common.

Also, an isNull check is more efficient than isEmpty, so I suggest using that instead.

> Source/WebCore/platform/network/ResourceResponseBase.h:141
> -    void platformLazyInit() { }
> +    void platformLazyInit(InitLevel initLevel) { UNUSED_PARAM(initLevel); }

Rather than using UNUSED_PARAM you can just leave the argument name out, since the class name makes it clear what it is.

> Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:46
> +    CFSTR("Age", CFSTR("Cache-Control", CFSTR("Date", CFSTR("Etag", CFSTR("Expires", CFSTR("Last-Modified", CFSTR("Pragma")

Missing end parentheses here mean this probably won’t compile.

[Build Bot]

Attachment 87725 [details] did not build on win:
Build output: http://queues.webkit.org/results/8309380

[Build Bot]

Attachment 87725 [details] did not build on win:
Build output: http://queues.webkit.org/results/8314330

[Darin Adler]

Comment on attachment 87725 [details]
Patch with update in response to reviewer's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=87725&action=review

review- since this doesn’t compile on Windows.

Otherwise, the only place with some room for improvement is with the comments and logic in the function to fetch header fields.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:253
> +    // Optimistic check first

You can probably omit this comment. If you did want to leave a comment you could have it address why we think this is a good idea. I think the rationale is that this prevents fetching a common field with a non-empty value from causing initialization of all fields. Putting that in writing could make it clear that we also want to optimize fetching a common field with an empty value.

> Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:88
> +    if ((m_initLevel < CommonFieldsOnly) && (initLevel >= CommonFieldsOnly)) {

I think this reads better without the extra parentheses.

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:49
> +static const NSString* commonHeaderFields[] = {

The const here is in the wrong place. It needs to be after the "*".

[Michael Saboff]

Created attachment 87793 [details]
Updated patch with windows build fix and changes in response to reviews.

[Build Bot]

Attachment 87793 [details] did not build on win:
Build output: http://queues.webkit.org/results/8314432

[Michael Saboff]

Created attachment 87802 [details]
Fixed missing closing paren

[Darin Adler]

Comment on attachment 87793 [details]
Updated patch with windows build fix and changes in response to reviews.

View in context: https://bugs.webkit.org/attachment.cgi?id=87793&action=review

Looks great! Still won’t compile on Windows but I’m going to say review+ as long as you fix the Windows build issue before checking in.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:253
> +    // Check to see if we already have the field before copying all header fields

I still think this comment focuses too much on what we’re doing, and not enough on why it’s a good idea. But it’s better and I’m not going to insist on further refinement.

Also, in WebKit we normally put a period on these sentence-style comments.

> Source/WebCore/platform/network/ResourceResponseBase.h:129
> +        Uninitialized = 0,

I don’t think you need this "= 0". For one thing, it will be 0. For another, who cares if it’s 0 or not?

> Source/WebCore/platform/network/cf/ResourceResponseCFNet.cpp:46
> +    CFSTR("Age", CFSTR("Cache-Control"), CFSTR("Date"), CFSTR("Etag"), CFSTR("Expires"), CFSTR("Last-Modified"), CFSTR("Pragma")

Still missing one parenthesis here, so it still won’t build!

[Build Bot]

Attachment 87802 [details] did not build on win:
Build output: http://queues.webkit.org/results/8315307

[Michael Saboff]

Created attachment 87810 [details]
Really fixed missing paren.  More feedback changes.

[Build Bot]

Attachment 87810 [details] did not build on win:
Build output: http://queues.webkit.org/results/8307202

[Michael Saboff]

Created attachment 87871 [details]
Fix windows build with cast

[Darin Adler]

Comment on attachment 87871 [details]
Fix windows build with cast

View in context: https://bugs.webkit.org/attachment.cgi?id=87871&action=review

> Source/WebCore/platform/network/ResourceResponseBase.cpp:267
> +    // Check to see if we already have a the field before copying all header fields

Stray “a” here.

[Michael Saboff]

Committed r82711: <http://trac.webkit.org/changeset/82711>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82711 might have broken Windows 7 Release (Tests)

[Michael Saboff]

Reopened to fix breakage on Windows.

[Michael Saboff]

Created attachment 87949 [details]
Fix for Windows test break - put header field access inside if (httpResponse) check

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 87949 [details]:

http/tests/appcache/fail-on-update-2.html bug 57691 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 87949 [details]
Fix for Windows test break - put header field access inside if (httpResponse) check

Rejecting attachment 87949 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 1

Last 500 characters of output:
900b2defebbf (refs/remotes/trunk)
	M	Source/WebKit2/ChangeLog
	M	Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp
	M	Source/WebKit2/UIProcess/win/WebPopupMenuProxyWin.h
	M	Source/WebKit2/UIProcess/WebPageProxy.h
	M	Source/WebKit2/UIProcess/WebPageProxy.messages.in
	M	Source/WebKit2/WebProcess/WebCoreSupport/WebPopupMenu.cpp
r82763 = d9db6c99df3a6e955e8132274ac1e525d06c5cde (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/8321383

[Michael Saboff]

Committed r82764: <http://trac.webkit.org/changeset/82764>