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.
Created attachment 239828 [details] Patch
Comment on attachment 239828 [details] Patch r=me
Comment on attachment 239828 [details] Patch Clearing flags on attachment: 239828 Committed r174717: <http://trac.webkit.org/changeset/174717>
All reviewed patches have been landed. Closing bug.
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 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.
(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*
Created attachment 239879 [details] Follow-up patch for nits.
Reopening to fix nits.
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.
(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 on attachment 239879 [details] Follow-up patch for nits. Clearing flags on attachment: 239879 Committed r174747: <http://trac.webkit.org/changeset/174747>