Bug 83180 - Make something faster than JSStringBuilder for joining an array of JSValue
: Make something faster than JSStringBuilder for joining an array of JSValue
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 83243
:
  Show dependency treegraph
 
Reported: 2012-04-04 10:30 PST by
Modified: 2012-04-05 12:30 PST (History)


Attachments
Patch (17.64 KB, patch)
2012-04-04 11:01 PST, Benjamin Poulain
no flags Review Patch | Details | Formatted Diff | Diff
Patch (20.59 KB, patch)
2012-04-04 11:22 PST, Benjamin Poulain
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.15 KB, patch)
2012-04-04 14:04 PST, Benjamin Poulain
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.08 KB, patch)
2012-04-04 15:06 PST, Benjamin Poulain
ggaren: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-04 10:30:16 PST
Joining a string is a special case of string builder. We can do a better job if we make strong assumptions.
------- Comment #1 From 2012-04-04 11:01:29 PST -------
Created an attachment (id=135629) [details]
Patch
------- Comment #2 From 2012-04-04 11:07:14 PST -------
This is my first step to make Array.join() faster.

Next I want to work on the problem of conversion UString->JSString->UString coming from JSValue::toString() (which is a regression.)

This version makes join() about 23% faster than the previous version.  Adding the special case of the FIXME adds a few percents but is more complex so I'll leave that for later if needed.
------- Comment #3 From 2012-04-04 11:10:40 PST -------
(From update of attachment 135629 [details])
Attachment 135629 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12319997
------- Comment #4 From 2012-04-04 11:15:09 PST -------
(From update of attachment 135629 [details])
I was so eager to commit that I forgot to update the other build systems...
------- Comment #5 From 2012-04-04 11:22:56 PST -------
Created an attachment (id=135638) [details]
Patch
------- Comment #6 From 2012-04-04 13:42:05 PST -------
(From update of attachment 135638 [details])
Attachment 135638 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12328067
------- Comment #7 From 2012-04-04 13:51:50 PST -------
../../../lib/libjavascriptcore_efl.a(ArrayPrototype.cpp.o): In function `JSC::arrayProtoFuncJoin(JSC::ExecState*)':
ArrayPrototype.cpp:(.text+0x6355): undefined reference to `JSC::JSStringJoiner::build(JSC::ExecState*)'
------- Comment #8 From 2012-04-04 14:04:16 PST -------
Created an attachment (id=135674) [details]
Patch
------- Comment #9 From 2012-04-04 14:09:47 PST -------
> Source/JavaScriptCore/runtime/JSStringJoiner.cpp:76
> +static inline PassRefPtr<StringImpl> joinStrings(Vector<UString>& strings, const UString& separator, unsigned outputLength)

strings can be a const Vector&.

> Source/JavaScriptCore/runtime/JSStringJoiner.cpp:90
> +        UString& string = strings.at(i);

I prefer "strings[i]" -style notation.

> Source/JavaScriptCore/runtime/JSStringJoiner.cpp:115
> +    is8Bits = is8Bits && m_separator.is8Bit();
> +    for (size_t i = 0; i < m_strings.size(); ++i) {

I think it would be slightly better to accumulate this data in a data member during append(). That will save you having to walk m_strings twice.

> Source/JavaScriptCore/runtime/JSStringJoiner.h:60
> +    m_isValid = m_strings.tryAppend(&str, 1);

I would change this to:

if (!m_isValid)
    return;
m_strings.uncheckedAppend(str);

Append should always succeed if you've pre-reserved capacity, so this makes a little more sense, and is a little more efficient.
------- Comment #10 From 2012-04-04 15:06:21 PST -------
Created an attachment (id=135694) [details]
Patch
------- Comment #11 From 2012-04-04 15:09:39 PST -------
> > Source/JavaScriptCore/runtime/JSStringJoiner.cpp:115
> > +    is8Bits = is8Bits && m_separator.is8Bit();
> > +    for (size_t i = 0; i < m_strings.size(); ++i) {
> 
> I think it would be slightly better to accumulate this data in a data member during append(). That will save you having to walk m_strings twice.
> 
> > Source/JavaScriptCore/runtime/JSStringJoiner.h:60
> > +    m_isValid = m_strings.tryAppend(&str, 1);
> 
> I would change this to:
> 
> if (!m_isValid)
>     return;
> m_strings.uncheckedAppend(str);
> 
> Append should always succeed if you've pre-reserved capacity, so this makes a little more sense, and is a little more efficient.

I updated following your ideas.

To my surprise, that does not change anything in the benchmarks. The profile was dominated with memory operations so I guess a few branches do not make much of a difference here.
------- Comment #12 From 2012-04-04 21:57:11 PST -------
(From update of attachment 135694 [details])
r=me
------- Comment #13 From 2012-04-05 12:30:42 PST -------
Committed r113355: <http://trac.webkit.org/changeset/113355>