Bug 73794 - Update KURL's copy copyASCII to avoid String::characters()
Summary: Update KURL's copy copyASCII to avoid String::characters()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks: 73928
  Show dependency treegraph
 
Reported: 2011-12-04 20:09 PST by Benjamin Poulain
Modified: 2011-12-06 09:52 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.35 KB, patch)
2011-12-04 20:15 PST, Benjamin Poulain
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2011-12-04 20:09:56 PST
String::characters() unnecessarily converts the characters to 16bits.
Comment 1 Benjamin Poulain 2011-12-04 20:15:30 PST
Created attachment 117828 [details]
Patch
Comment 2 Andreas Kling 2011-12-04 20:58:39 PST
Comment on attachment 117828 [details]
Patch

Neat! r=me
Comment 3 WebKit Review Bot 2011-12-04 21:45:21 PST
Comment on attachment 117828 [details]
Patch

Attachment 117828 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10734563

New failing tests:
media/event-attributes.html
Comment 4 Benjamin Poulain 2011-12-05 21:48:44 PST
The test media/event-attributes.html is flaky, it has been blacklisted in 101850.
I'll land this manually
Comment 5 Benjamin Poulain 2011-12-05 22:12:41 PST
Committed r102094: <http://trac.webkit.org/changeset/102094>
Comment 6 Darin Adler 2011-12-06 09:33:03 PST
Comment on attachment 117828 [details]
Patch

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

> Source/WebCore/platform/KURL.cpp:261
> +    if (string.isNull())
> +        return;

Why does null need a special case? If it needs one, why doesn’t empty need one?

> Source/WebCore/platform/KURL.cpp:268
> +        for (size_t i = 0; i < string.length(); i++)
> +            dest[i] = static_cast<char>(src[i]);

Since this calls String::length() every time through the loop, I believe this may be slower than the old loop you were replacing. The length() function its not entirely trivial.
Comment 7 Benjamin Poulain 2011-12-06 09:37:23 PST
(In reply to comment #6)
> > Source/WebCore/platform/KURL.cpp:261
> > +    if (string.isNull())
> > +        return;
> 
> Why does null need a special case? If it needs one, why doesn’t empty need one?

You cannot call is8Bit() on a null string. I figured isEmpty() would be clearer so I asked on IRC to change isNull() to isEmpty() (that's the patch that landed).

> > Source/WebCore/platform/KURL.cpp:268
> > +        for (size_t i = 0; i < string.length(); i++)
> > +            dest[i] = static_cast<char>(src[i]);
> 
> Since this calls String::length() every time through the loop, I believe this may be slower than the old loop you were replacing. The length() function its not entirely trivial.

That is right, I'll fix that.
Comment 8 Andreas Kling 2011-12-06 09:38:00 PST
Comment on attachment 117828 [details]
Patch

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

>> Source/WebCore/platform/KURL.cpp:261
>> +        return;
> 
> Why does null need a special case? If it needs one, why doesn’t empty need one?

Benjamin and I talked about this on IRC, and it was landed with isEmpty() rather than isNull().

>> Source/WebCore/platform/KURL.cpp:268
>> +            dest[i] = static_cast<char>(src[i]);
> 
> Since this calls String::length() every time through the loop, I believe this may be slower than the old loop you were replacing. The length() function its not entirely trivial.

Fudge! I missed that one, my bad. I suspect the compiler might very well move the nullity check of m_impl out of the loop though.
But it would indeed be better to put it in a local.