Bug 185986 – URL::host should return a StringView to reduce allocations

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

URL::host should return a StringView to reduce allocations

[Alex Christensen]

Created attachment 341305 [details]
Patch

[Alex Christensen]

Created attachment 341308 [details]
Patch

[Alex Christensen]

Created attachment 341310 [details]
Patch

[Geoffrey Garen]

Comment on attachment 341310 [details]
Patch

In file included from /home/ews-wpe/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource382.cpp:8:0:
../../Source/WebCore/platform/network/soup/SoupNetworkSession.cpp: In static member function ‘static std::optional<WebCore::ResourceError> WebCore::SoupNetworkSession::checkTLSErrors(const WebCore::URL&, GTlsCertificate*, GTlsCertificateFlags)’:
../../Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:280:58: error: no matching function for call to ‘WTF::HashMap<WTF::String, WebCore::HostTLSCertificateSet, WTF::ASCIICaseInsensitiveHash>::find(WTF::StringView)’
     auto it = clientCertificates().find(requestURL.host());
                                                          ^

[Geoffrey Garen]

Plz fix build before landing.

[Sam Weinig]

Comment on attachment 341310 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadStatistics.cpp:336
> -    return primaryDomain(url.host());
> +    return primaryDomain(url.host().toString());

Seems like the ResourceLoadStatistics::primaryDomain() that takes a String could be converted to take a StringView, avoiding the allocation in some cases.

> Source/WebCore/platform/network/cf/SocketStreamHandleImplCFNet.cpp:315
> +    RetainPtr<CFStringRef> host = m_url.host().toString().createCFString();

Seems unfortunate to do two allocations here, one for toString(), one for createCFString(). Can we add a createCFString() to StringView to avoid the extraneous allocation?

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:248
> +    String requestHost(requestUrlObj.host().toString().convertToASCIILowercase());

Seems unfortunate to do two allocations here, one for toString(), one for convertToASCIILowercase(). Can we add a convertToASCIILowercase() to StringView to avoid the extraneous allocation?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:4852
> +    auto host = url.host();
>      return equalLettersIgnoringASCIICase(host, "docs.google.com");

I think avoiding the local would be nicer here.

> Tools/TestWebKitAPI/Tests/WebCore/URL.cpp:62
> -    EXPECT_EQ(String("www.example.com"), kurl.host());
> +    EXPECT_EQ(String("www.example.com"), kurl.host().toString());

lol, kurl.

[Alex Christensen]

Created attachment 341311 [details]
Patch

[Alex Christensen]

(In reply to Sam Weinig from comment #6)
This change makes those existing problems more obvious and fixable while improving or preserving the status quo.  I'll do some of those as a follow-up.

[Alex Christensen]

Created attachment 341317 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r232198

[Radar WebKit Bug Importer]

<rdar://problem/40564881>

[Darin Adler]

Comment on attachment 341310 [details]
Patch

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

This is a great idea, but risky when it’s called on the result of a function that returns a URL. Or on any URL that might have a lifetime shorter than the StringView of the host. It’s hard to spot such situations by code inspection of the patch, but I did not spot any.

> Source/WebCore/loader/mac/LoaderNSURLExtras.mm:51
> +        NSString *host = URL(url).host().toString();

Should use WTF::StringView::createNSString to convert directly from WTF::StringView to NSString without a temporary WTF::String.

> Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:68
> +    return equalIgnoringASCIICase(host, m_host) || (m_hostHasWildcard && host.endsWithIgnoringASCIICase(makeString(".", m_host)));

More efficient to use '.' instead of "." in the call to makeString. But even more efficient to write a function that can do this match without constructing a string. Not super-hard to do that.

> Source/WebCore/platform/URL.cpp:183
> +        return makeString(host(), ':', String::number(port.value()));

More efficient to find a way to make StringConcatenateNumbers.h do the numeric conversion here as part of the makeString call instead of using WTF::String::number; avoids creating and destroying a String. Slightly tricky because the port number is the type that happens to be the same as UChar, but it can be done by putting the port into an int/int32_t for now and eventually we can figure out how to disambiguate UChar and uint16_t.

>> Source/WebCore/platform/network/cf/SocketStreamHandleImplCFNet.cpp:315
>> +    RetainPtr<CFStringRef> host = m_url.host().toString().createCFString();
> 
> Seems unfortunate to do two allocations here, one for toString(), one for createCFString(). Can we add a createCFString() to StringView to avoid the extraneous allocation?

We definitely can do that. WTF::StringView already has both createNSString and createCFStringWithoutCopying. It’s trivial to write a createCFString too.

>> Source/WebCore/platform/network/curl/CookieJarDB.cpp:248
>> +    String requestHost(requestUrlObj.host().toString().convertToASCIILowercase());
> 
> Seems unfortunate to do two allocations here, one for toString(), one for convertToASCIILowercase(). Can we add a convertToASCIILowercase() to StringView to avoid the extraneous allocation?

This idea applies to at least 4 other cases above in the patch too.

[Alex Christensen]

I'm addressing Darin's feedback in https://bugs.webkit.org/show_bug.cgi?id=186059

[Alexey Proskuryakov]

Looks like this broke Windows build: https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/9652/steps/compile-webkit/logs/stdio

How could a change like this be landed without all bots green on EWS?

[Alexey Proskuryakov]

Attempted build fix in 232292, let's see how it goes.

[Alex Christensen]

*** Bug 173192 has been marked as a duplicate of this bug. ***