Bug 67342 – Replace local implementation of string equals() methods with UString versions

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

As part of the changes for https://bugs.webkit.org/show_bug.cgi?id=66161 with the goal of eliminating the use of ::characters() calls, we can coalesce the various equals() methods that exist in the code and have those call sites use the UString implementations.

[Michael Saboff]

Created attachment 106003 [details]
Proposed patch

[Michael Saboff]

Created attachment 106007 [details]
Real proposed patch

[Darin Adler]

Comment on attachment 106007 [details]
Real proposed patch

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

> Source/JavaScriptCore/runtime/Identifier.cpp:72
> +    return WTF::equal(r, s);

It looks like there is an additional level of function call here. What’s the performance impact?

> Source/JavaScriptCore/runtime/Identifier.h:140
> +        return WTF::equal(r, s, length);

It looks like there is an additional level of function call here as well as additional null checks. What’s the performance impact?

> Source/JavaScriptCore/wtf/text/AtomicString.cpp:93
> +    static inline bool equal(StringImpl* r, const char* s)

The inline keyword is redundant inside a class definition. There is no value to adding it.

> Source/JavaScriptCore/wtf/text/AtomicString.cpp:95
> +        return WTF::equal(r, s);

It looks like there is an additional level of function call here as well as additional null checks. What’s the performance impact?

> Source/JavaScriptCore/wtf/text/AtomicString.cpp:108
> +    return WTF::equal(a.impl(), b);

It looks like there is an additional level of function call here. What’s the performance impact?

> Source/JavaScriptCore/wtf/text/AtomicString.cpp:-138
> -static inline bool equal(StringImpl* string, const UChar* characters, unsigned length)

Looks like the new version of the function is no longer inlined for callers within this source file. What is the performance impact of that?

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:1003
> +    if (!b)
> +        return !a;

This can be return false.

[Gavin Barraclough]

Comment on attachment 106007 [details]
Real proposed patch

Per our discussion, since the call sites to equal are all trivial methods we should be able to inline these instead, removing the additional layer of function calling & likely overall reducing the size of the binary.

r+ with Darin's comments + perf testing (particularly for JSC, since you're changing the implementation of equal for Identifiers).

[Michael Saboff]

Made changes suggested by Darin and Gavin.

Measured performance with updated changes:

                           Baseline       w/updated changes
SunSpider command line   230.0 +/- 0.2%    230.0 +/- 0.2%
V8 command line         1639.4 +/- 0.3%   1638.2 +/- 0.2%
Dromaeo DOM Core        1494.69           1499.86

[Michael Saboff]

Committed r94475: <http://trac.webkit.org/changeset/94475>