Bug 137721 – [Mac] Fix inefficiencies in ResourceResponse::platformLazyInit(InitLevel)

• Component: Platform
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 239828 [details]
Patch

[Geoffrey Garen]

Comment on attachment 239828 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 239828 [details]
Patch

Clearing flags on attachment: 239828

Committed r174717: <http://trac.webkit.org/changeset/174717>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Chris Dumez]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 239879 [details]
Follow-up patch for nits.

[Chris Dumez]

Reopening to fix nits.

[Darin Adler]

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.

[Chris Dumez]

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

[WebKit Commit Bot]

Comment on attachment 239879 [details]
Follow-up patch for nits.

Clearing flags on attachment: 239879

Committed r174747: <http://trac.webkit.org/changeset/174747>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.