Bug 158827 – Avoid some temporary String allocations for common HTTP headers in ResourceResponse::platformLazyInit()

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Chris Dumez]

Avoid some temporary String allocations for common HTTP headers in ResourceResponse::platformLazyInit() by adding a HTTPHeaderMap::set() overload taking a CFStringRef in.

[Chris Dumez]

Created attachment 281435 [details]
Patch

[Darin Adler]

Comment on attachment 281435 [details]
Patch

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

> Source/WebCore/platform/network/HTTPHeaderMap.cpp:69
> +    if (const char* nameCharacters = CFStringGetCStringPtr(name, kCFStringEncodingMacRoman)) {

I suggest auto* here.

After doing a little research on the behavior of recent versions of CF, I suggest using kCFStringEncodingASCII instead of kCFStringEncodingMacRoman here.

> Source/WebCore/platform/network/HTTPHeaderMap.cpp:70
> +        unsigned length =  CFStringGetLength(name);

Extra space here after the "=".

> Source/WebCore/platform/network/HTTPHeaderMap.cpp:75
> +            m_uncommonHeaders.set(String(nameCharacters, length), value);

This line of code is incorrect for some non-ASCII characters. The nameCharacters might contain non-Latin-1 characters that are part of kCFStringEncodingMacRoman, and this will convert them into different characters. Easiest fix for that is probably to pass kCFStringEncodingASCII instead. Other fixes involve scanning the characters to see if they are all ASCII.

[Chris Dumez]

Created attachment 281461 [details]
Patch

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 281435 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281435&action=review
> 
> > Source/WebCore/platform/network/HTTPHeaderMap.cpp:69
> > +    if (const char* nameCharacters = CFStringGetCStringPtr(name, kCFStringEncodingMacRoman)) {
> 
> I suggest auto* here.
> 
> After doing a little research on the behavior of recent versions of CF, I
> suggest using kCFStringEncodingASCII instead of kCFStringEncodingMacRoman
> here.
> 
> > Source/WebCore/platform/network/HTTPHeaderMap.cpp:70
> > +        unsigned length =  CFStringGetLength(name);
> 
> Extra space here after the "=".
> 
> > Source/WebCore/platform/network/HTTPHeaderMap.cpp:75
> > +            m_uncommonHeaders.set(String(nameCharacters, length), value);
> 
> This line of code is incorrect for some non-ASCII characters. The
> nameCharacters might contain non-Latin-1 characters that are part of
> kCFStringEncodingMacRoman, and this will convert them into different
> characters. Easiest fix for that is probably to pass kCFStringEncodingASCII
> instead. Other fixes involve scanning the characters to see if they are all
> ASCII.

Made the changes. I have confirmed that the hit rate is still around 93% when using kCFStringEncodingASCII.

[WebKit Commit Bot]

Comment on attachment 281461 [details]
Patch

Clearing flags on attachment: 281461

Committed r202126: <http://trac.webkit.org/changeset/202126>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

Would it work to make the overload take a StringView, instead of the CFStringRef/NSString, and have callers convert to StringView if they have non-String types? Then we could keep the platform specific type out of HTTPHeaderMap?

[Chris Dumez]

(In reply to comment #7)
> Would it work to make the overload take a StringView, instead of the
> CFStringRef/NSString, and have callers convert to StringView if they have
> non-String types? Then we could keep the platform specific type out of
> HTTPHeaderMap?

I wanted to do this at first. However,
1. The converting of a CFStringRef / NSString* into a StringView is not trivial because it is not always possible to get a pointer to the internal characters of a CFStringRef / NSString*.
2. There are 4 call sites (ResourceRequest / ResourceResponse in Cocoa and CF flavors).

I felt like duplicating the relatively complex code for converting a CFStringRef / NSString* into a StringView at 4 different places was not very nice.

What do you think?

Alternatively, I also thought about adding a StringView constructor that takes a CFStringRef in like so:
StringView(CFStringRef string, std::unique_ptr<char[]>& characters);

Which would construct a StringView from the internal CFStringRef characters when possible and would otherwise fallback to extracting those characters and transfer ownership of the characters to the caller via the second parameter.

It would reduce complexity at the 4 call sites.

[Darin Adler]

(In reply to comment #8)
> Alternatively, I also thought about adding a StringView constructor that
> takes a CFStringRef in like so:
> StringView(CFStringRef string, std::unique_ptr<char[]>& characters);
> 
> Which would construct a StringView from the internal CFStringRef characters
> when possible and would otherwise fallback to extracting those characters
> and transfer ownership of the characters to the caller via the second
> parameter.
> 
> It would reduce complexity at the 4 call sites.

If we decide to do something like this, I suggest making it a class instead of a StringView constructor. Call the class something like CocoaStringView; just as with the String class, it needs to outlive any StringView that is made from it. It would usually be fast to destroy, but allocates storage on the heap if needed. Might have some inline capacity too so short strings still don’t require heap allocation even when GetCStringPtr fails.

[Chris Dumez]

Confirmed 1% PLT progression on iOS :)