Summary: | URL::host should return a StringView to reduce allocations | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ap, darin, ggaren, sam, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Alex Christensen
2018-05-25 11:23:20 PDT
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
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. *** |