Bug 138079 - Use separate HashMaps for common and uncommon headers in HTTPHeaderMap
Summary: Use separate HashMaps for common and uncommon headers in HTTPHeaderMap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 138164
Blocks:
  Show dependency treegraph
 
Reported: 2014-10-25 21:32 PDT by Chris Dumez
Modified: 2014-10-28 20:14 PDT (History)
8 users (show)

See Also:


Attachments
Patch (30.69 KB, patch)
2014-10-25 22:07 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.06 KB, patch)
2014-10-27 09:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.15 KB, patch)
2014-10-27 10:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.18 KB, patch)
2014-10-27 12:18 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.24 KB, patch)
2014-10-27 13:26 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-25 21:32:23 PDT
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())
Comment 1 Chris Dumez 2014-10-25 22:07:36 PDT
Created attachment 240461 [details]
Patch
Comment 2 Anders Carlsson 2014-10-27 07:40:19 PDT
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.
Comment 3 Chris Dumez 2014-10-27 09:29:23 PDT
Created attachment 240483 [details]
Patch
Comment 4 Chris Dumez 2014-10-27 10:57:18 PDT
Created attachment 240488 [details]
Patch
Comment 5 Anders Carlsson 2014-10-27 11:41:16 PDT
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.
Comment 6 Chris Dumez 2014-10-27 11:47:26 PDT
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".
Comment 7 Chris Dumez 2014-10-27 12:18:28 PDT
Created attachment 240491 [details]
Patch
Comment 8 Chris Dumez 2014-10-27 12:19:22 PDT
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.
Comment 9 Anders Carlsson 2014-10-27 12:23:36 PDT
(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.
Comment 10 Anders Carlsson 2014-10-27 12:24:34 PDT
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&
Comment 11 Chris Dumez 2014-10-27 13:26:05 PDT
Created attachment 240497 [details]
Patch
Comment 12 WebKit Commit Bot 2014-10-27 14:08:47 PDT
Comment on attachment 240497 [details]
Patch

Clearing flags on attachment: 240497

Committed r175231: <http://trac.webkit.org/changeset/175231>
Comment 13 WebKit Commit Bot 2014-10-27 14:08:52 PDT
All reviewed patches have been landed.  Closing bug.