Bug 162241 - Reduce allocations in URLParser
Summary: Reduce allocations in URLParser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
: 162045 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-09-19 15:31 PDT by Alex Christensen
Modified: 2016-10-03 16:03 PDT (History)
1 user (show)

See Also:


Attachments
Patch (22.39 KB, patch)
2016-09-19 15:39 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (16.13 KB, patch)
2016-09-19 16:40 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (15.87 KB, patch)
2016-09-20 14:51 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-09-19 15:31:55 PDT
Reduce allocations in URLParser
Comment 1 Alex Christensen 2016-09-19 15:39:20 PDT
Created attachment 289267 [details]
Patch
Comment 2 Chris Dumez 2016-09-19 16:07:30 PDT
Comment on attachment 289267 [details]
Patch

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

Sorry, I did not have time to do a proper review yet. Just a few comments.

> Source/WebCore/ChangeLog:9
> +        1. When URLParsing fails, use the original String for things like href.

Do we really care about optimizing the failure case?

> Source/WebCore/platform/URLParser.cpp:873
> +        return parse(input, input.characters8(), input.length(), base, encoding);

Passing input in addition to the characters is a bit odd. Since it seems to be used only to optimize the failure case, I am not sure it is worth it.

> Source/WebCore/platform/URLParser.cpp:1543
> +        for (size_t i = 0; i < m_asciiBuffer.size(); ++i)

There is an appendVector().

> Source/WebCore/platform/URLParser.cpp:1546
> +            buffer.uncheckedAppend(m_unicodeFragmentBuffer[i]);

There is an appendVector().
Comment 3 Alex Christensen 2016-09-19 16:40:23 PDT
Created attachment 289280 [details]
Patch
Comment 4 Chris Dumez 2016-09-20 14:18:30 PDT
Comment on attachment 289280 [details]
Patch

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

r=me

> Source/WebCore/platform/URLParser.cpp:115
> +static void appendCodePoint(UChar32 codePoint, Vector<UChar>& destination)

I think it may look better if we reversed those parameters.

> Source/WebCore/platform/URLParser.cpp:1925
> +            for (size_t i = 0; i < domain.length(); i++)

++i

> Source/WebCore/platform/URLParser.cpp:1926
> +                ascii.uncheckedAppend(toASCIILower(domain.characters8()[i]));

Please cache characters8() before the loop

> Source/WebCore/platform/URLParser.cpp:1929
> +        Vector<LChar, defaultInlineBufferSize> ascii;

Can we refactor so that there is only 1 vector and 1 return statement?

> Source/WebCore/platform/URLParser.cpp:1933
> +            ascii.uncheckedAppend(toASCIILower(domain.characters16()[i]));

Please cache characters16() before the loop
Comment 5 Alex Christensen 2016-09-20 14:51:45 PDT
Created attachment 289402 [details]
Patch
Comment 6 Alex Christensen 2016-09-20 14:53:04 PDT
http://trac.webkit.org/changeset/206177
Comment 7 Alex Christensen 2016-10-03 16:03:21 PDT
*** Bug 162045 has been marked as a duplicate of this bug. ***