URL::host should return a StringView to reduce allocations
Created attachment 341305 [details] Patch
Created attachment 341308 [details] Patch
Created attachment 341310 [details] Patch
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()); ^
Plz fix build before landing.
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.
Created attachment 341311 [details] Patch
(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.
Created attachment 341317 [details] Patch
http://trac.webkit.org/r232198
<rdar://problem/40564881>
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.
I'm addressing Darin's feedback in https://bugs.webkit.org/show_bug.cgi?id=186059
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?
Attempted build fix in 232292, let's see how it goes.
*** Bug 173192 has been marked as a duplicate of this bug. ***