Bug 162089 – Remove unnecessary String allocations in URLParser

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Remove unnecessary String allocations in URLParser

[Alex Christensen]

Created attachment 289112 [details]
Patch

[Alex Christensen]

Created attachment 289116 [details]
Patch

[Chris Dumez]

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.

[Alex Christensen]

Created attachment 289117 [details]
Patch

[Alex Christensen]

Created attachment 289122 [details]
Patch

[Chris Dumez]

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.

[Alex Christensen]

(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.

[Alex Christensen]

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.

[Chris Dumez]

(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.

[Chris Dumez]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Alex Christensen]

Created attachment 289147 [details]
Patch

[Alex Christensen]

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.

[Chris Dumez]

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.

[Alex Christensen]

Created attachment 289193 [details]
Patch

[WebKit Commit Bot]

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

[Alex Christensen]

Created attachment 289195 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/changeset/206076