| Summary: | Remove unnecessary String allocations in URLParser | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, cdumez, commit-queue, darin, dbates, japhet, rniwa, sam | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Alex Christensen
2016-09-16 14:21:08 PDT
Created attachment 289112 [details]
Patch
Created attachment 289116 [details]
Patch
Comment on attachment 289116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289116&action=review > Source/WebCore/platform/URL.h:131 > + bool protocolIs(const char* protocol) const { return protocolIs(protocol, strlen(protocol)); } The strlen() here is a bit sad. Created attachment 289117 [details]
Patch
Created attachment 289122 [details]
Patch
Comment on attachment 289122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289122&action=review I am not a fan of the end result and I fear the resulting code complexity is not worth the optimization. One thought I have that *may* look nicer if you really want this optimization would be to add a URL::protocolIs(const LChar*, size_t) with your efficient implementation in there. And leave the rest of the code alone (i.e. not update bool URL::protocolIs(const char*) to call yours. > Source/WebCore/loader/PolicyChecker.cpp:124 > + if (!request.isNull() && request.url().protocolIs(protocol.data(), protocol.size())) { Why is this better? > Source/WebCore/loader/ResourceLoadNotifier.cpp:120 > + if (!request.isNull() && request.url().protocolIs(protocol.data(), protocol.size())) Why is this better? > Source/WebCore/loader/cache/CachedResource.cpp:292 > + if (!m_resourceRequest.isNull() && m_resourceRequest.url().protocolIs(protocol.data(), protocol.size())) { Why is this better? > Source/WebCore/platform/SchemeRegistry.cpp:67 > + secureSchemes.get().add(String(reinterpret_cast<const LChar*>(QLPreviewProtocol().data()), QLPreviewProtocol().size())); Why is this better? > Source/WebCore/platform/URL.h:133 > + bool protocolIs(const char* protocol) const { return protocolIs(protocol, strlen(protocol)); } This overload is effectively slower than it used to. > Source/WebCore/platform/URLParser.cpp:951 > + if (base.protocolIs(reinterpret_cast<const char*>(m_asciiBuffer.data()), m_asciiBuffer.size())) So all this code re-shuffling (in most cases making the code less concise) is only to optimize this single call site as far as I can tell. > Source/WebCore/platform/network/ios/QuickLook.h:73 > +WEBCORE_EXPORT const Vector<char>& QLPreviewProtocol(); It is odd for such API to return a (null terminated) Vector. (In reply to comment #6) > > Source/WebCore/platform/URL.h:133 > > + bool protocolIs(const char* protocol) const { return protocolIs(protocol, strlen(protocol)); } > > This overload is effectively slower than it used to. Shoot, I meant to remove this overload. Will remove. > > > Source/WebCore/platform/URLParser.cpp:951 > > + if (base.protocolIs(reinterpret_cast<const char*>(m_asciiBuffer.data()), m_asciiBuffer.size())) > > So all this code re-shuffling (in most cases making the code less concise) > is only to optimize this single call site as far as I can tell. That is a very important call site. This code removes an allocation and string copy for every URL that is parsed with a special scheme, which is most of them. > > > Source/WebCore/platform/network/ios/QuickLook.h:73 > > +WEBCORE_EXPORT const Vector<char>& QLPreviewProtocol(); > > It is odd for such API to return a (null terminated) Vector. Oops, I'll make it non-null-terminated. Comment on attachment 289122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289122&action=review >> Source/WebCore/loader/PolicyChecker.cpp:124 >> + if (!request.isNull() && request.url().protocolIs(protocol.data(), protocol.size())) { > > Why is this better? This is exactly equivalent to what used to be there, but it allows me to remove that string allocation. (In reply to comment #7) > (In reply to comment #6) > > > Source/WebCore/platform/URL.h:133 > > > + bool protocolIs(const char* protocol) const { return protocolIs(protocol, strlen(protocol)); } > > > > This overload is effectively slower than it used to. > Shoot, I meant to remove this overload. Will remove. > > > > > Source/WebCore/platform/URLParser.cpp:951 > > > + if (base.protocolIs(reinterpret_cast<const char*>(m_asciiBuffer.data()), m_asciiBuffer.size())) > > > > So all this code re-shuffling (in most cases making the code less concise) > > is only to optimize this single call site as far as I can tell. > That is a very important call site. This code removes an allocation and > string copy for every URL that is parsed with a special scheme, which is > most of them. > > > > > Source/WebCore/platform/network/ios/QuickLook.h:73 > > > +WEBCORE_EXPORT const Vector<char>& QLPreviewProtocol(); > > > > It is odd for such API to return a (null terminated) Vector. > Oops, I'll make it non-null-terminated. That is not the issue. Returning a Vector altogether is weird. Comment on attachment 289122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289122&action=review >>> Source/WebCore/loader/PolicyChecker.cpp:124 >>> + if (!request.isNull() && request.url().protocolIs(protocol.data(), protocol.size())) { >> >> Why is this better? > > This is exactly equivalent to what used to be there, but it allows me to remove that string allocation. What string allocation? it used to call protocolIs(char*). That overload would not construct a string and would iterate over the characters only once. The new code may be as efficient but this definitely does not look as good. Comment on attachment 289122 [details] Patch Attachment 289122 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2090706 Number of test failures exceeded the failure limit. Created attachment 289134 [details]
Archive of layout-test-results from ews112 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 289122 [details] Patch Attachment 289122 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2090750 Number of test failures exceeded the failure limit. Created attachment 289138 [details]
Archive of layout-test-results from ews101 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 289122 [details] Patch Attachment 289122 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2090818 Number of test failures exceeded the failure limit. Created attachment 289144 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 289147 [details]
Patch
Comment on attachment 289147 [details]
Patch
Ok, here's a much less invasive change that doesn't touch much existing code and removes some string allocations.
Comment on attachment 289147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289147&action=review r=me with changes > Source/WebCore/platform/URL.cpp:828 > +bool URL::protocolIs(const char* protocol, size_t length) const I would prefer const LChar* > Source/WebCore/platform/URL.cpp:830 > + assertProtocolIsGood(protocol, strlen(protocol)); strlen(protocol) -> length > Source/WebCore/platform/URL.h:132 > + bool protocolIs(const char*, size_t) const; I would prefer const LChar* here. > Source/WebCore/platform/URLParser.cpp:-949 > - m_asciiBuffer.append(':'); You don't need to move this (and therefore duplicate this). Just use m_asciiBuffer.size() - 1 when you call protocolIs(). > Source/WebCore/platform/URLParser.cpp:951 > + if (base.protocolIs(reinterpret_cast<const char*>(m_asciiBuffer.data()), m_asciiBuffer.size())) You don't need this reinterpret_cast if you update the method to take a const LChar* as I suggested. Created attachment 289193 [details]
Patch
Comment on attachment 289193 [details] Patch Rejecting attachment 289193 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ild/Release/WebCore.build/Objects-normal/x86_64/UserAgent.dia -c /Volumes/Data/EWS/WebKit/Source/WebCore/page/cocoa/UserAgent.mm -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/UserAgent.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/URL.o platform/URL.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: http://webkit-queues.webkit.org/results/2098242 Created attachment 289195 [details]
Patch
|