Bug 53711 - Remove duplicated code from AtomicString::fromUTF8()
Summary: Remove duplicated code from AtomicString::fromUTF8()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-03 13:20 PST by Patrick R. Gansterer
Modified: 2011-04-10 15:07 PDT (History)
4 users (show)

See Also:


Attachments
Patch (9.34 KB, patch)
2011-02-03 13:22 PST, Patrick R. Gansterer
paroga: review-
Details | Formatted Diff | Diff
Patch (9.41 KB, patch)
2011-02-03 15:09 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (9.49 KB, patch)
2011-03-19 05:29 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2011-02-03 13:20:48 PST
Remove duplicated code from AtomicString::fromUTF8()
Comment 1 Patrick R. Gansterer 2011-02-03 13:22:22 PST
Created attachment 81106 [details]
Patch
Comment 2 Build Bot 2011-02-03 15:02:45 PST
Attachment 81106 [details] did not build on win:
Build output: http://queues.webkit.org/results/7693522
Comment 3 Patrick R. Gansterer 2011-02-03 15:09:53 PST
Created attachment 81119 [details]
Patch
Comment 4 Build Bot 2011-02-03 15:57:02 PST
Attachment 81106 [details] did not build on win:
Build output: http://queues.webkit.org/results/7689800
Comment 5 Patrick R. Gansterer 2011-02-08 10:37:44 PST
@darin: ping
Comment 6 David Levin 2011-02-08 17:42:34 PST
Comment on attachment 81119 [details]
Patch

r- b/c this causes a efl build break. Ideally that would be fixed as long as it is possible.
Comment 7 Patrick R. Gansterer 2011-03-19 05:29:11 PDT
Created attachment 86263 [details]
Patch
Comment 8 Darin Adler 2011-03-28 09:46:51 PDT
Comment on attachment 86263 [details]
Patch

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

> Source/JavaScriptCore/wtf/text/AtomicString.h:182
> +inline AtomicString AtomicString::fromUTF8(const char* characters, size_t length)
> +{
> +    if (!characters)
> +        return nullAtom;
> +    if (!length)
> +        return emptyAtom;
> +    return fromUTF8Internal(characters, characters + length);
> +}
> +
> +inline AtomicString AtomicString::fromUTF8(const char* characters)
> +{
> +    if (!characters)
> +        return nullAtom;
> +    if (!*characters)
> +        return emptyAtom;
> +    return fromUTF8Internal(characters, 0);
> +}

Inlining more like this is a big deal. It will probably make code size a bit bigger because of more checks at every single callsite. When called with a constant C literal, I think the code might get constant-folded away, which would be great. I’m not sure about making this change without checking if there is performance impact.
Comment 9 Patrick R. Gansterer 2011-03-28 09:53:40 PDT
(In reply to comment #8)
> When called with a constant C literal, I think the code might get constant-folded away, which would be great.
I'm not sure if we use UTF-8 strings in the code. ;-)

> I’m not sure about making this change without checking if there is performance impact.
If I remeber correctly it was a slight performance increase, but I'll test it again and post the values here before landing.
Comment 10 Patrick R. Gansterer 2011-04-10 12:34:45 PDT
Comment on attachment 86263 [details]
Patch

(In reply to comment #9)
> > I’m not sure about making this change without checking if there is performance impact.
> If I remeber correctly it was a slight performance increase, but I'll test it again and post the values here before landing.
I'v run the xml-parser performance test and this change seams like a 1% performance improvement, but the variation of the results is too big to verify if that's true. :-)
Comment 11 WebKit Commit Bot 2011-04-10 15:07:54 PDT
Comment on attachment 86263 [details]
Patch

Clearing flags on attachment: 86263

Committed r83407: <http://trac.webkit.org/changeset/83407>
Comment 12 WebKit Commit Bot 2011-04-10 15:07:58 PDT
All reviewed patches have been landed.  Closing bug.