Bug 83180 – Make something faster than JSStringBuilder for joining an array of JSValue

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

[Benjamin Poulain]

Joining a string is a special case of string builder. We can do a better job if we make strong assumptions.

[Benjamin Poulain]

Created attachment 135629 [details]
Patch

[Benjamin Poulain]

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.

[Build Bot]

Comment on attachment 135629 [details]
Patch

Attachment 135629 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12319997

[Benjamin Poulain]

Comment on attachment 135629 [details]
Patch

I was so eager to commit that I forgot to update the other build systems...

[Benjamin Poulain]

Created attachment 135638 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 135638 [details]
Patch

Attachment 135638 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12328067

[Geoffrey Garen]

../../../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*)'

[Benjamin Poulain]

Created attachment 135674 [details]
Patch

[Geoffrey Garen]

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

[Benjamin Poulain]

Created attachment 135694 [details]
Patch

[Benjamin Poulain]

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

[Geoffrey Garen]

Comment on attachment 135694 [details]
Patch

r=me

[Benjamin Poulain]

Committed r113355: <http://trac.webkit.org/changeset/113355>