Bug 137721 - [Mac] Fix inefficiencies in ResourceResponse::platformLazyInit(InitLevel)
Summary: [Mac] Fix inefficiencies in ResourceResponse::platformLazyInit(InitLevel)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Enhancement
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-14 15:05 PDT by Chris Dumez
Modified: 2014-10-15 17:10 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.25 KB, patch)
2014-10-14 15:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Follow-up patch for nits. (2.33 KB, patch)
2014-10-15 11:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-10-14 15:05:30 PDT
There are several inefficiencies with the current Mac implementation of ResourceResponse::platformLazyInit(InitLevel):
1. We end up initializing uncommon fields even if called with CommonFieldsOnly initLevel.
2. If called with AllFields initLevel (if currently uninitialized), we end up populating m_httpHeaderFields twice, once with only the common headers, then with ALL the headers. We can skip the common-header case in this case to avoid wasting CPU time.
Comment 1 Chris Dumez 2014-10-14 15:23:24 PDT
Created attachment 239828 [details]
Patch
Comment 2 Geoffrey Garen 2014-10-14 17:02:32 PDT
Comment on attachment 239828 [details]
Patch

r=me
Comment 3 WebKit Commit Bot 2014-10-14 17:44:48 PDT
Comment on attachment 239828 [details]
Patch

Clearing flags on attachment: 239828

Committed r174717: <http://trac.webkit.org/changeset/174717>
Comment 4 WebKit Commit Bot 2014-10-14 17:46:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Darin Adler 2014-10-15 09:30:35 PDT
Comment on attachment 239828 [details]
Patch

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

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:141
> +                for (unsigned i = 0; i < WTF_ARRAY_LENGTH(commonHeaderFields); ++i) {

This could use a modern C++ for loop and avoid the need to use WTF_ARRAY_LENGTH.

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:143
> +                        m_httpHeaderFields.set(String(commonHeaderFields[i]), headerValue);

I’m surprised we need the explicit String() here. Also, it would be a bit more efficient to create this String if we used ASCIILiteral. And in additional, HashMap::add generates slightly less code than set, which is an add followed by an optional assignment, and here we could use either, so I suggest we use add.

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:164
>                  m_httpHeaderFields.set(String(name), [headers objectForKey:name]);

I’m surprised we need the explicit String() here.
Comment 6 Chris Dumez 2014-10-15 11:10:48 PDT
Comment on attachment 239828 [details]
Patch

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

>> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:141
>> +                for (unsigned i = 0; i < WTF_ARRAY_LENGTH(commonHeaderFields); ++i) {
> 
> This could use a modern C++ for loop and avoid the need to use WTF_ARRAY_LENGTH.

Right.

>> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:143
>> +                        m_httpHeaderFields.set(String(commonHeaderFields[i]), headerValue);
> 
> I’m surprised we need the explicit String() here. Also, it would be a bit more efficient to create this String if we used ASCIILiteral. And in additional, HashMap::add generates slightly less code than set, which is an add followed by an optional assignment, and here we could use either, so I suggest we use add.

I don't think ASCIILiteral works here. Right now, we have an array of NSString*, so using a modern loop would look like for (NSString* name : commonHeaderFields). I cannot call ASCIILIteral(NSString*). It is an NSString* because it is used in [headers objectForKey:name].

Regarding the second comment. The issue is that m_httpHeaderFields is not a HashMap, it is an HTTPHeaderMap. HTTPHeaderMap::add() actually does more work because if the key is already present, it will *append* the new value to the existing value. So, either way, we end up checking !result.isNewEntry (which is never true because we clear the HashMap before this loop) and we return without further value change. In the end it is equivalent performance-wise but I think HTTPHeaderMap::add() is clearer here because we really want to set the header value, not append the new value to the existing one.

>> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:164
>>                  m_httpHeaderFields.set(String(name), [headers objectForKey:name]);
> 
> I’m surprised we need the explicit String() here.

We don't. As a matter of fact, if that constructor was explicit, we would need String([headers objectForKey:name]) as well.

Also note that we could not use HTTPHeaderMap::add() here because common headers might already be in the HashMap and we would end up appending / duplicating the value, instead of overwriting it.
Comment 7 Chris Dumez 2014-10-15 11:13:18 PDT
(In reply to comment #6)
> (From update of attachment 239828 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239828&action=review
> 
> >> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:141
> >> +                for (unsigned i = 0; i < WTF_ARRAY_LENGTH(commonHeaderFields); ++i) {
> > 
> > This could use a modern C++ for loop and avoid the need to use WTF_ARRAY_LENGTH.
> 
> Right.
> 
> >> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:143
> >> +                        m_httpHeaderFields.set(String(commonHeaderFields[i]), headerValue);
> > 
> > I’m surprised we need the explicit String() here. Also, it would be a bit more efficient to create this String if we used ASCIILiteral. And in additional, HashMap::add generates slightly less code than set, which is an add followed by an optional assignment, and here we could use either, so I suggest we use add.
> 
> I don't think ASCIILiteral works here. Right now, we have an array of NSString*, so using a modern loop would look like for (NSString* name : commonHeaderFields). I cannot call ASCIILIteral(NSString*). It is an NSString* because it is used in [headers objectForKey:name].
> 
> Regarding the second comment. The issue is that m_httpHeaderFields is not a HashMap, it is an HTTPHeaderMap. HTTPHeaderMap::add() actually does more work because if the key is already present, it will *append* the new value to the existing value. So, either way, we end up checking !result.isNewEntry (which is never true because we clear the HashMap before this loop) and we return without further value change. In the end it is equivalent performance-wise but I think HTTPHeaderMap::add() is clearer here because we really want to set the header value, not append the new value to the existing one.

typo: I meant "HTTPHeaderMap::set() is clearer*
Comment 8 Chris Dumez 2014-10-15 11:16:28 PDT
Created attachment 239879 [details]
Follow-up patch for nits.
Comment 9 Chris Dumez 2014-10-15 12:59:07 PDT
Reopening to fix nits.
Comment 10 Darin Adler 2014-10-15 16:15:08 PDT
Comment on attachment 239828 [details]
Patch

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

>>> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:143
>>> +                        m_httpHeaderFields.set(String(commonHeaderFields[i]), headerValue);
>> 
>> I’m surprised we need the explicit String() here. Also, it would be a bit more efficient to create this String if we used ASCIILiteral. And in additional, HashMap::add generates slightly less code than set, which is an add followed by an optional assignment, and here we could use either, so I suggest we use add.
> 
> I don't think ASCIILiteral works here. Right now, we have an array of NSString*, so using a modern loop would look like for (NSString* name : commonHeaderFields). I cannot call ASCIILIteral(NSString*). It is an NSString* because it is used in [headers objectForKey:name].
> 
> Regarding the second comment. The issue is that m_httpHeaderFields is not a HashMap, it is an HTTPHeaderMap. HTTPHeaderMap::add() actually does more work because if the key is already present, it will *append* the new value to the existing value. So, either way, we end up checking !result.isNewEntry (which is never true because we clear the HashMap before this loop) and we return without further value change. In the end it is equivalent performance-wise but I think HTTPHeaderMap::add() is clearer here because we really want to set the header value, not append the new value to the existing one.

Makes sense, my comment was about HashMap::set, not about HTTPHeaderMap::set!

If the performance of this function matters enough, we should consider a separate array containing each of the common header field names in WTF::String form; seems wasteful to convert from NSString to WTF::String each time.
Comment 11 Chris Dumez 2014-10-15 16:24:31 PDT
(In reply to comment #10)
> (From update of attachment 239828 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239828&action=review
> 
> >>> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:143
> >>> +                        m_httpHeaderFields.set(String(commonHeaderFields[i]), headerValue);
> >> 
> >> I’m surprised we need the explicit String() here. Also, it would be a bit more efficient to create this String if we used ASCIILiteral. And in additional, HashMap::add generates slightly less code than set, which is an add followed by an optional assignment, and here we could use either, so I suggest we use add.
> > 
> > I don't think ASCIILiteral works here. Right now, we have an array of NSString*, so using a modern loop would look like for (NSString* name : commonHeaderFields). I cannot call ASCIILIteral(NSString*). It is an NSString* because it is used in [headers objectForKey:name].
> > 
> > Regarding the second comment. The issue is that m_httpHeaderFields is not a HashMap, it is an HTTPHeaderMap. HTTPHeaderMap::add() actually does more work because if the key is already present, it will *append* the new value to the existing value. So, either way, we end up checking !result.isNewEntry (which is never true because we clear the HashMap before this loop) and we return without further value change. In the end it is equivalent performance-wise but I think HTTPHeaderMap::add() is clearer here because we really want to set the header value, not append the new value to the existing one.
> 
> Makes sense, my comment was about HashMap::set, not about HTTPHeaderMap::set!
> 
> If the performance of this function matters enough, we should consider a separate array containing each of the common header field names in WTF::String form; seems wasteful to convert from NSString to WTF::String each time.

I don't think it is worth it. Based on my investigation, we mostly call this function with AllFields initLevel (rarely CommonFieldsOnly). This is usually triggered by serialization for the IPC between the NetworkProcess and the WebProcess. FYI, what is hot in this function right now is:

m_url = [m_nsResponse.get() URL]; // 7.1%
m_mimeType = [m_nsResponse.get() MIMEType]; // 7.1%
m_expectedContentLength = [m_nsResponse.get() expectedContentLength]; // 7.1%
m_textEncodingName = [m_nsResponse.get() textEncodingName]; // 28.6%

if (RetainPtr<NSString> httpStatusLine = adoptNS(wkCopyNSURLResponseStatusLine(httpResponse))) // 7.1%

for (NSString *name in headers) // 7.1%
    m_httpHeaderFields.set(String(name), [headers objectForKey:name]); // 21.4%
[pool drain]; // 7.1%
Comment 12 WebKit Commit Bot 2014-10-15 17:10:03 PDT
Comment on attachment 239879 [details]
Follow-up patch for nits.

Clearing flags on attachment: 239879

Committed r174747: <http://trac.webkit.org/changeset/174747>
Comment 13 WebKit Commit Bot 2014-10-15 17:10:08 PDT
All reviewed patches have been landed.  Closing bug.