Bug 138079 – Use separate HashMaps for common and uncommon headers in HTTPHeaderMap

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Dumez]

Use separate HashMaps for common and uncommon headers in HTTPHeaderMap. The common header map would be a HashMap<HTTPHeaderMap, String> and the uncommon header one would be a HashMap<String, String, CaseFoldingHash>.

There are potential several advantages:
- Creating cross-thread data requires creating less isolated String copies
- Should speed up deserialization for IPC as it will avoid having to call findHTTPHeaderName() for every deserialized header. We will already know if it is a common header or not, and if it is a common header, we will already have a HTTPHeaderName instead of a String
- The hash table performance should be slightly better for common headers due to faster hashing and faster key comparison in case of collision.
- Avoids having to construct String objects from HTTPHeaderMap values for storing.
- Some call sites can benefit from having direct access to common headers, in HTTPHeaderMap type (e.g. CrossOriginAccessControl::isOnAccessControlSimpleRequestHeaderWhitelist(), CachedRawResource::shouldIgnoreHeaderForCacheReuse())

[Chris Dumez]

Created attachment 240461 [details]
Patch

[Anders Carlsson]

Comment on attachment 240461 [details]
Patch

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

I really like the direction of this patch; it's something I wanted to do back when I got rid of the atomic strings.

The problem I see with this patch is that it makes it more complex to iterate headers. I think we still want to be able to iterate all headers and just have the iterator be able to return an enum (if it's a common header) or a string.

> Source/WTF/wtf/HashTraits.h:93
> +    using UnderLyingType = typename std::underlying_type<T>::type;

UnderLying should be Underlying.

[Chris Dumez]

Created attachment 240483 [details]
Patch

[Chris Dumez]

Created attachment 240488 [details]
Patch

[Anders Carlsson]

Comment on attachment 240488 [details]
Patch

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

> Source/WebCore/platform/network/HTTPHeaderMap.h:50
> +    typedef HashMap<HTTPHeaderName, String, WTF::IntHash<HTTPHeaderName>, WTF::StrongEnumHashTraits<HTTPHeaderName>> CommonHeadersHashMapType;
> +    typedef HashMap<String, String, CaseFoldingHash> UncommonHeadersHashMapType;

I think the "Type" suffix is redundant here.

> Source/WebCore/platform/network/HTTPHeaderMap.h:65
> +            bool keyIsCommonHeader;
> +            HTTPHeaderName keyAsHTTPHeaderName;

Maybe instead of having these two fields, the keyAsHTTPHeaderName could be a WTF::Optional<HTTPHeaderName>? That would make it harder to accidentally read keyAsHTTPHeaderName if it's not a common header name.

> Source/WebCore/platform/network/HTTPHeaderMap.h:92
> +            return m_commonHeadersIt == other.m_commonHeadersIt && m_uncommonHeadersIt == other.m_uncommonHeadersIt;

I think you want to check m_table here as well.

[Chris Dumez]

Comment on attachment 240488 [details]
Patch

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

>> Source/WebCore/platform/network/HTTPHeaderMap.h:50
>> +    typedef HashMap<String, String, CaseFoldingHash> UncommonHeadersHashMapType;
> 
> I think the "Type" suffix is redundant here.

Ok, just followed the previous HashMapType scheme but I agree with you.

>> Source/WebCore/platform/network/HTTPHeaderMap.h:65
>> +            HTTPHeaderName keyAsHTTPHeaderName;
> 
> Maybe instead of having these two fields, the keyAsHTTPHeaderName could be a WTF::Optional<HTTPHeaderName>? That would make it harder to accidentally read keyAsHTTPHeaderName if it's not a common header name.

Ok, I actually did not know about WTF::Optional but I will take a look.

>> Source/WebCore/platform/network/HTTPHeaderMap.h:92
>> +            return m_commonHeadersIt == other.m_commonHeadersIt && m_uncommonHeadersIt == other.m_uncommonHeadersIt;
> 
> I think you want to check m_table here as well.

Assuming the HashMap iterators already make sure the iterators are for the same HashMap. Isn't that sufficient? Meaning:
if "CommonHashMap is the same" + if "UncommonHashMap is the same" then "HTTPHeaderMap is the same".

[Chris Dumez]

Created attachment 240491 [details]
Patch

[Chris Dumez]

Comment on attachment 240488 [details]
Patch

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

>>> Source/WebCore/platform/network/HTTPHeaderMap.h:50
>>> +    typedef HashMap<String, String, CaseFoldingHash> UncommonHeadersHashMapType;
>> 
>> I think the "Type" suffix is redundant here.
> 
> Ok, just followed the previous HashMapType scheme but I agree with you.

Done.

>>> Source/WebCore/platform/network/HTTPHeaderMap.h:65
>>> +            HTTPHeaderName keyAsHTTPHeaderName;
>> 
>> Maybe instead of having these two fields, the keyAsHTTPHeaderName could be a WTF::Optional<HTTPHeaderName>? That would make it harder to accidentally read keyAsHTTPHeaderName if it's not a common header name.
> 
> Ok, I actually did not know about WTF::Optional but I will take a look.

Done.

>>> Source/WebCore/platform/network/HTTPHeaderMap.h:92
>>> +            return m_commonHeadersIt == other.m_commonHeadersIt && m_uncommonHeadersIt == other.m_uncommonHeadersIt;
>> 
>> I think you want to check m_table here as well.
> 
> Assuming the HashMap iterators already make sure the iterators are for the same HashMap. Isn't that sufficient? Meaning:
> if "CommonHashMap is the same" + if "UncommonHashMap is the same" then "HTTPHeaderMap is the same".

Not done yet as per my comment.

[Anders Carlsson]

(In reply to comment #8)
> > Assuming the HashMap iterators already make sure the iterators are for the same HashMap. Isn't that sufficient? Meaning:
> > if "CommonHashMap is the same" + if "UncommonHashMap is the same" then "HTTPHeaderMap is the same".
> 
> Not done yet as per my comment.

Makes sense.

[Anders Carlsson]

Comment on attachment 240491 [details]
Patch

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

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:471
> +    for (auto& keyValuePair : headerMap.commonHeaders()) {

I think this can be const auto&

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:476
> +    for (auto& keyValuePair : headerMap.uncommonHeaders()) {

I think this can be const auto&

[Chris Dumez]

Created attachment 240497 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 240497 [details]
Patch

Clearing flags on attachment: 240497

Committed r175231: <http://trac.webkit.org/changeset/175231>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.