Bug 57488

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

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.

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.

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

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

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.

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

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

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 "*".

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

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

Created attachment 87802 [details] Fixed missing closing paren

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!

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

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

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

Created attachment 87871 [details] Fix windows build with cast

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.

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

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

Reopened to fix breakage on Windows.

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

The commit-queue encountered the following flaky tests while processing attachment 87949 [details]: http/tests/appcache/fail-on-update-2.html bug 57691 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

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

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