Bug 53711 – Remove duplicated code from AtomicString::fromUTF8()

• Component: New Bugs
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

Remove duplicated code from AtomicString::fromUTF8()

[Patrick R. Gansterer]

Created attachment 81106 [details]
Patch

[Build Bot]

Attachment 81106 [details] did not build on win:
Build output: http://queues.webkit.org/results/7693522

[Patrick R. Gansterer]

Created attachment 81119 [details]
Patch

[Build Bot]

Attachment 81106 [details] did not build on win:
Build output: http://queues.webkit.org/results/7689800

[Patrick R. Gansterer]

@darin: ping

[David Levin]

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.

[Patrick R. Gansterer]

Created attachment 86263 [details]
Patch

[Darin Adler]

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.

[Patrick R. Gansterer]

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

[Patrick R. Gansterer]

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. :-)

[WebKit Commit Bot]

Comment on attachment 86263 [details]
Patch

Clearing flags on attachment: 86263

Committed r83407: <http://trac.webkit.org/changeset/83407>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.