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())
Created attachment 240461 [details] Patch
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.
Created attachment 240483 [details] Patch
Created attachment 240488 [details] Patch
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 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".
Created attachment 240491 [details] Patch
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.
(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 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&
Created attachment 240497 [details] Patch
Comment on attachment 240497 [details] Patch Clearing flags on attachment: 240497 Committed r175231: <http://trac.webkit.org/changeset/175231>
All reviewed patches have been landed. Closing bug.