Bug 67342 - Replace local implementation of string equals() methods with UString versions
Summary: Replace local implementation of string equals() methods with UString versions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 66661
  Show dependency treegraph
 
Reported: 2011-08-31 16:53 PDT by Michael Saboff
Modified: 2011-09-02 18:13 PDT (History)
0 users

See Also:


Attachments
Proposed patch (9.22 KB, patch)
2011-09-01 12:15 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Real proposed patch (7.36 KB, patch)
2011-09-01 12:22 PDT, Michael Saboff
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2011-08-31 16:53:39 PDT
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.
Comment 1 Michael Saboff 2011-09-01 12:15:22 PDT
Created attachment 106003 [details]
Proposed patch
Comment 2 Michael Saboff 2011-09-01 12:22:19 PDT
Created attachment 106007 [details]
Real proposed patch
Comment 3 Darin Adler 2011-09-01 13:47:43 PDT
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.
Comment 4 Gavin Barraclough 2011-09-02 13:25:49 PDT
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).
Comment 5 Michael Saboff 2011-09-02 16:50:27 PDT
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
Comment 6 Michael Saboff 2011-09-02 18:13:33 PDT
Committed r94475: <http://trac.webkit.org/changeset/94475>