Changeset 190882 in webkit


Ignore:
Timestamp:
Oct 12, 2015 12:44:52 PM (9 years ago)
Author:
akling@apple.com
Message:

"A + B" with strings shouldn't copy if A or B is empty.
<​https://webkit.org/b/150034>

Reviewed by Anders Carlsson.

Source/JavaScriptCore:

  • runtime/JSStringBuilder.h:

(JSC::jsMakeNontrivialString):

  • runtime/Lookup.cpp:

(JSC::reifyStaticAccessor):

  • runtime/ObjectPrototype.cpp:

(JSC::objectProtoFuncToString):

Source/WTF:

Add a fast path to WTF's operator+ magic for concatenation of two strings where
one of them is empty. In that case, try to avoid allocation altogether by returning
the non-empty string.

Spotted this while analyzing memory peaks during page load; it turns out we were
duplicating whole text resources (JS, CSS) at the end of decoding, below
TextResourceDecoder::decodeAndFlush(). That function effectively does:

return decode() + flush();

Very often, flush() returns an empty string, so due to the naive operator+,
we'd allocate a new StringImpl of length (decode().length() + flush().length())
and copy the return value from decode() into it. So silly!

Had to make the tryMakeString() machinery use String as a return type instead of
RefPtr<StringImpl> to make all the right overloads gel. StringTypeAdapter templates
are now required to provide a toString() function.

  • wtf/text/StringConcatenate.h:

(WTF::StringTypeAdapter<char>::toString):
(WTF::StringTypeAdapter<UChar>::toString):
(WTF::StringTypeAdapter<Vector<char>>::toString):
(WTF::StringTypeAdapter<String>::toString):
(WTF::tryMakeString):
(WTF::makeString):

  • wtf/text/StringOperators.h:

(WTF::StringAppend::operator String):

  • wtf/text/StringView.h:

(WTF::StringTypeAdapter<StringView>::toString):

Location:
trunk/Source
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r190881 r190882  
     12015-10-12  Andreas Kling  <akling@apple.com>
     2
     3        "A + B" with strings shouldn't copy if A or B is empty.
     4        <https://webkit.org/b/150034>
     5
     6        Reviewed by Anders Carlsson.
     7
     8        * runtime/JSStringBuilder.h:
     9        (JSC::jsMakeNontrivialString):
     10        * runtime/Lookup.cpp:
     11        (JSC::reifyStaticAccessor):
     12        * runtime/ObjectPrototype.cpp:
     13        (JSC::objectProtoFuncToString):
     14
    1152015-10-12  Joseph Pecoraro  <pecoraro@apple.com>
    216

  • trunk/Source/JavaScriptCore/runtime/JSStringBuilder.h

    r174219 r190882  
    128128inline JSValue jsMakeNontrivialString(ExecState* exec, const StringType& string, const StringTypes&... strings)
    129129{
    130     RefPtr<StringImpl> result = WTF::tryMakeString(string, strings...);
     130    String result = WTF::tryMakeString(string, strings...);
    131131    if (!result)
    132132        return throwOutOfMemoryError(exec);
    133     return jsNontrivialString(exec, result.release());
     133    return jsNontrivialString(exec, result);
    134134}
    135135

  • trunk/Source/JavaScriptCore/runtime/Lookup.cpp

    r190305 r190882  
    3333    GetterSetter* accessor = GetterSetter::create(vm, globalObject);
    3434    if (value.accessorGetter()) {
    35         RefPtr<StringImpl> getterName = WTF::tryMakeString(ASCIILiteral("get "), String(*propertyName.publicName()));
     35        String getterName = WTF::tryMakeString(ASCIILiteral("get "), String(*propertyName.publicName()));
    3636        if (!getterName)
    3737            return;
    3838        accessor->setGetter(vm, globalObject, value.attributes() & Builtin
    39             ? JSFunction::createBuiltinFunction(vm, value.builtinAccessorGetterGenerator()(vm), globalObject, *getterName)
    40             : JSFunction::create(vm, globalObject, 0, *getterName, value.accessorGetter()));
     39            ? JSFunction::createBuiltinFunction(vm, value.builtinAccessorGetterGenerator()(vm), globalObject, getterName)
     40            : JSFunction::create(vm, globalObject, 0, getterName, value.accessorGetter()));
    4141    }
    4242    thisObj.putDirectNonIndexAccessor(vm, propertyName, accessor, attributesForStructure(value.attributes()));

  • trunk/Source/JavaScriptCore/runtime/ObjectPrototype.cpp

    r183535 r190882  
    246246    JSString* result = thisObject->structure(vm)->objectToStringValue();
    247247    if (!result) {
    248         RefPtr<StringImpl> newString = WTF::tryMakeString("[object ", thisObject->methodTable(exec->vm())->className(thisObject), "]");
     248        String newString = WTF::tryMakeString("[object ", thisObject->methodTable(exec->vm())->className(thisObject), "]");
    249249        if (!newString)
    250250            return JSValue::encode(throwOutOfMemoryError(exec));
    251251
    252         result = jsNontrivialString(&vm, newString.release());
     252        result = jsNontrivialString(&vm, newString);
    253253        thisObject->structure(vm)->setObjectToStringValue(vm, result);
    254254    }

  • trunk/Source/WTF/ChangeLog

    r190875 r190882  
     12015-10-12  Andreas Kling  <akling@apple.com>
     2
     3        "A + B" with strings shouldn't copy if A or B is empty.
     4        <https://webkit.org/b/150034>
     5
     6        Reviewed by Anders Carlsson.
     7
     8        Add a fast path to WTF's operator+ magic for concatenation of two strings where
     9        one of them is empty. In that case, try to avoid allocation altogether by returning
     10        the non-empty string.
     11
     12        Spotted this while analyzing memory peaks during page load; it turns out we were
     13        duplicating whole text resources (JS, CSS) at the end of decoding, below
     14        TextResourceDecoder::decodeAndFlush(). That function effectively does:
     15
     16            return decode() + flush();
     17
     18        Very often, flush() returns an empty string, so due to the naive operator+,
     19        we'd allocate a new StringImpl of length (decode().length() + flush().length())
     20        and copy the return value from decode() into it. So silly!
     21
     22        Had to make the tryMakeString() machinery use String as a return type instead of
     23        RefPtr<StringImpl> to make all the right overloads gel. StringTypeAdapter templates
     24        are now required to provide a toString() function.
     25
     26        * wtf/text/StringConcatenate.h:
     27        (WTF::StringTypeAdapter<char>::toString):
     28        (WTF::StringTypeAdapter<UChar>::toString):
     29        (WTF::StringTypeAdapter<Vector<char>>::toString):
     30        (WTF::StringTypeAdapter<String>::toString):
     31        (WTF::tryMakeString):
     32        (WTF::makeString):
     33        * wtf/text/StringOperators.h:
     34        (WTF::StringAppend::operator String):
     35        * wtf/text/StringView.h:
     36        (WTF::StringTypeAdapter<StringView>::toString):
     37
    1382015-10-12  Filip Pizlo  <fpizlo@apple.com>
    239

  • trunk/Source/WTF/wtf/text/StringConcatenate.h

    r187875 r190882  
    11/*
    2  * Copyright (C) 2010 Apple Inc. All rights reserved.
     2 * Copyright (C) 2010-2015 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    6969    }
    7070
     71    String toString() const { return String(&m_character, 1); }
     72
    7173private:
    7274    char m_character;
    … …  
    9496        *destination = m_character;
    9597    }
     98
     99    String toString() const { return String(&m_character, 1); }
    96100
    97101private:
    … …  
    121125    }
    122126
     127    String toString() const { return String(m_characters, 1); }
     128
    123129private:
    124130    const LChar* m_characters;
    … …  
    155161    }
    156162
     163    String toString() const { return String(m_characters, 1); }
     164
    157165private:
    158166    const UChar* m_characters;
    … …  
    208216    }
    209217
     218    String toString() const { return String(m_vector.data(), m_vector.size()); }
     219
    210220private:
    211221    const Vector<char>& m_vector;
    … …  
    234244        WTF_STRINGTYPEADAPTER_COPIED_WTF_STRING();
    235245    }
     246
     247    String toString() const { return m_string; }
    236248
    237249private:
    … …  
    257269
    258270template<typename StringType1, typename StringType2>
    259 RefPtr<StringImpl> tryMakeString(StringType1 string1, StringType2 string2)
     271String tryMakeString(StringType1 string1, StringType2 string2)
    260272{
    261273    StringTypeAdapter<StringType1> adapter1(string1);
    262274    StringTypeAdapter<StringType2> adapter2(string2);
     275
     276    if (adapter1.length() && !adapter2.length())
     277        return adapter1.toString();
     278    if (!adapter1.length() && adapter2.length())
     279        return adapter2.toString();
    263280
    264281    bool overflow = false;
    … …  
    266283    sumWithOverflow(length, adapter2.length(), overflow);
    267284    if (overflow)
    268         return nullptr;
     285        return String();
    269286
    270287    if (adapter1.is8Bit() && adapter2.is8Bit()) {
    … …  
    272289        RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    273290        if (!resultImpl)
    274             return nullptr;
     291            return String();
    275292
    276293        LChar* result = buffer;
    … …  
    285302    RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    286303    if (!resultImpl)
    287         return nullptr;
     304        return String();
    288305
    289306    UChar* result = buffer;
    … …  
    296313
    297314template<typename StringType1, typename StringType2, typename StringType3>
    298 RefPtr<StringImpl> tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3)
     315String tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3)
    299316{
    300317    StringTypeAdapter<StringType1> adapter1(string1);
    … …  
    307324    sumWithOverflow(length, adapter3.length(), overflow);
    308325    if (overflow)
    309         return nullptr;
     326        return String();
    310327
    311328    if (adapter1.is8Bit() && adapter2.is8Bit() && adapter3.is8Bit()) {
    … …  
    313330        RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    314331        if (!resultImpl)
    315             return nullptr;
     332            return String();
    316333
    317334        LChar* result = buffer;
    … …  
    328345    RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    329346    if (!resultImpl)
    330         return nullptr;
     347        return String();
    331348
    332349    UChar* result = buffer;
    … …  
    341358
    342359template<typename StringType1, typename StringType2, typename StringType3, typename StringType4>
    343 RefPtr<StringImpl> tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4)
     360String tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4)
    344361{
    345362    StringTypeAdapter<StringType1> adapter1(string1);
    … …  
    354371    sumWithOverflow(length, adapter4.length(), overflow);
    355372    if (overflow)
    356         return nullptr;
     373        return String();
    357374
    358375    if (adapter1.is8Bit() && adapter2.is8Bit() && adapter3.is8Bit() && adapter4.is8Bit()) {
    … …  
    360377        RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    361378        if (!resultImpl)
    362             return nullptr;
     379            return String();
    363380
    364381        LChar* result = buffer;
    … …  
    377394    RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    378395    if (!resultImpl)
    379         return nullptr;
     396        return String();
    380397
    381398    UChar* result = buffer;
    … …  
    392409
    393410template<typename StringType1, typename StringType2, typename StringType3, typename StringType4, typename StringType5>
    394 RefPtr<StringImpl> tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5)
     411String tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5)
    395412{
    396413    StringTypeAdapter<StringType1> adapter1(string1);
    … …  
    407424    sumWithOverflow(length, adapter5.length(), overflow);
    408425    if (overflow)
    409         return nullptr;
     426        return String();
    410427
    411428    if (adapter1.is8Bit() && adapter2.is8Bit() && adapter3.is8Bit() && adapter4.is8Bit() && adapter5.is8Bit()) {
    … …  
    413430        RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    414431        if (!resultImpl)
    415             return nullptr;
     432            return String();
    416433
    417434        LChar* result = buffer;
    … …  
    432449    RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    433450    if (!resultImpl)
    434         return nullptr;
     451        return String();
    435452
    436453    UChar* result = buffer;
    … …  
    449466
    450467template<typename StringType1, typename StringType2, typename StringType3, typename StringType4, typename StringType5, typename StringType6>
    451 RefPtr<StringImpl> tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5, StringType6 string6)
     468String tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5, StringType6 string6)
    452469{
    453470    StringTypeAdapter<StringType1> adapter1(string1);
    … …  
    466483    sumWithOverflow(length, adapter6.length(), overflow);
    467484    if (overflow)
    468         return nullptr;
     485        return String();
    469486
    470487    if (adapter1.is8Bit() && adapter2.is8Bit() && adapter3.is8Bit() && adapter4.is8Bit() && adapter5.is8Bit() && adapter6.is8Bit()) {
    … …  
    472489        RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    473490        if (!resultImpl)
    474             return nullptr;
     491            return String();
    475492
    476493        LChar* result = buffer;
    … …  
    493510    RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    494511    if (!resultImpl)
    495         return nullptr;
     512        return String();
    496513
    497514    UChar* result = buffer;
    … …  
    512529
    513530template<typename StringType1, typename StringType2, typename StringType3, typename StringType4, typename StringType5, typename StringType6, typename StringType7>
    514 RefPtr<StringImpl> tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5, StringType6 string6, StringType7 string7)
     531String tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5, StringType6 string6, StringType7 string7)
    515532{
    516533    StringTypeAdapter<StringType1> adapter1(string1);
    … …  
    531548    sumWithOverflow(length, adapter7.length(), overflow);
    532549    if (overflow)
    533         return nullptr;
     550        return String();
    534551
    535552    if (adapter1.is8Bit() && adapter2.is8Bit() && adapter3.is8Bit() && adapter4.is8Bit() && adapter5.is8Bit() && adapter6.is8Bit() && adapter7.is8Bit()) {
    … …  
    537554        RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    538555        if (!resultImpl)
    539             return nullptr;
     556            return String();
    540557
    541558        LChar* result = buffer;
    … …  
    560577    RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    561578    if (!resultImpl)
    562         return nullptr;
     579        return String();
    563580
    564581    UChar* result = buffer;
    … …  
    581598
    582599template<typename StringType1, typename StringType2, typename StringType3, typename StringType4, typename StringType5, typename StringType6, typename StringType7, typename StringType8>
    583 RefPtr<StringImpl> tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5, StringType6 string6, StringType7 string7, StringType8 string8)
     600String tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5, StringType6 string6, StringType7 string7, StringType8 string8)
    584601{
    585602    StringTypeAdapter<StringType1> adapter1(string1);
    … …  
    602619    sumWithOverflow(length, adapter8.length(), overflow);
    603620    if (overflow)
    604         return nullptr;
     621        return String();
    605622
    606623    if (adapter1.is8Bit() && adapter2.is8Bit() && adapter3.is8Bit() && adapter4.is8Bit() && adapter5.is8Bit() && adapter6.is8Bit() && adapter7.is8Bit() && adapter8.is8Bit()) {
    … …  
    608625        RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    609626        if (!resultImpl)
    610             return nullptr;
     627            return String();
    611628
    612629        LChar* result = buffer;
    … …  
    633650    RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    634651    if (!resultImpl)
    635         return nullptr;
     652        return String();
    636653
    637654    UChar* result = buffer;
    … …  
    656673
    657674template<typename StringType1, typename StringType2, typename StringType3, typename StringType4, typename StringType5, typename StringType6, typename StringType7, typename StringType8, typename StringType9>
    658 RefPtr<StringImpl> tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5, StringType6 string6, StringType7 string7, StringType8 string8, StringType9 string9)
     675String tryMakeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5, StringType6 string6, StringType7 string7, StringType8 string8, StringType9 string9)
    659676{
    660677    StringTypeAdapter<StringType1> adapter1(string1);
    … …  
    679696    sumWithOverflow(length, adapter9.length(), overflow);
    680697    if (overflow)
    681         return nullptr;
     698        return String();
    682699
    683700    if (adapter1.is8Bit() && adapter2.is8Bit() && adapter3.is8Bit() && adapter4.is8Bit() && adapter5.is8Bit() && adapter6.is8Bit() && adapter7.is8Bit() && adapter8.is8Bit() && adapter9.is8Bit()) {
    … …  
    685702        RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    686703        if (!resultImpl)
    687             return nullptr;
     704            return String();
    688705
    689706        LChar* result = buffer;
    … …  
    712729    RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
    713730    if (!resultImpl)
    714         return nullptr;
     731        return String();
    715732
    716733    UChar* result = buffer;
    … …  
    747764String makeString(StringType1 string1, StringType2 string2)
    748765{
    749     RefPtr<StringImpl> resultImpl = tryMakeString(string1, string2);
    750     if (!resultImpl)
     766    String result = tryMakeString(string1, string2);
     767    if (!result)
    751768        CRASH();
    752     return WTF::move(resultImpl);
     769    return result;
    753770}
    754771
    … …  
    756773String makeString(StringType1 string1, StringType2 string2, StringType3 string3)
    757774{
    758     RefPtr<StringImpl> resultImpl = tryMakeString(string1, string2, string3);
    759     if (!resultImpl)
     775    String result = tryMakeString(string1, string2, string3);
     776    if (!result)
    760777        CRASH();
    761     return WTF::move(resultImpl);
     778    return result;
    762779}
    763780
    … …  
    765782String makeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4)
    766783{
    767     RefPtr<StringImpl> resultImpl = tryMakeString(string1, string2, string3, string4);
    768     if (!resultImpl)
     784    String result = tryMakeString(string1, string2, string3, string4);
     785    if (!result)
    769786        CRASH();
    770     return WTF::move(resultImpl);
     787    return result;
    771788}
    772789
    … …  
    774791String makeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5)
    775792{
    776     RefPtr<StringImpl> resultImpl = tryMakeString(string1, string2, string3, string4, string5);
    777     if (!resultImpl)
     793    String result = tryMakeString(string1, string2, string3, string4, string5);
     794    if (!result)
    778795        CRASH();
    779     return WTF::move(resultImpl);
     796    return result;
    780797}
    781798
    … …  
    783800String makeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5, StringType6 string6)
    784801{
    785     RefPtr<StringImpl> resultImpl = tryMakeString(string1, string2, string3, string4, string5, string6);
    786     if (!resultImpl)
     802    String result = tryMakeString(string1, string2, string3, string4, string5, string6);
     803    if (!result)
    787804        CRASH();
    788     return WTF::move(resultImpl);
     805    return result;
    789806}
    790807
    … …  
    792809String makeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5, StringType6 string6, StringType7 string7)
    793810{
    794     RefPtr<StringImpl> resultImpl = tryMakeString(string1, string2, string3, string4, string5, string6, string7);
    795     if (!resultImpl)
     811    String result = tryMakeString(string1, string2, string3, string4, string5, string6, string7);
     812    if (!result)
    796813        CRASH();
    797     return WTF::move(resultImpl);
     814    return result;
    798815}
    799816
    … …  
    801818String makeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5, StringType6 string6, StringType7 string7, StringType8 string8)
    802819{
    803     RefPtr<StringImpl> resultImpl = tryMakeString(string1, string2, string3, string4, string5, string6, string7, string8);
    804     if (!resultImpl)
     820    String result = tryMakeString(string1, string2, string3, string4, string5, string6, string7, string8);
     821    if (!result)
    805822        CRASH();
    806     return WTF::move(resultImpl);
     823    return result;
    807824}
    808825
    … …  
    810827String makeString(StringType1 string1, StringType2 string2, StringType3 string3, StringType4 string4, StringType5 string5, StringType6 string6, StringType7 string7, StringType8 string8, StringType9 string9)
    811828{
    812     RefPtr<StringImpl> resultImpl = tryMakeString(string1, string2, string3, string4, string5, string6, string7, string8, string9);
    813     if (!resultImpl)
     829    String result = tryMakeString(string1, string2, string3, string4, string5, string6, string7, string8, string9);
     830    if (!result)
    814831        CRASH();
    815     return WTF::move(resultImpl);
     832    return result;
    816833}
    817834

  • trunk/Source/WTF/wtf/text/StringOperators.h

    r187875 r190882  
    3636    operator String() const
    3737    {
    38         RefPtr<StringImpl> resultImpl = tryMakeString(m_string1, m_string2);
    39         if (!resultImpl)
     38        String result = tryMakeString(m_string1, m_string2);
     39        if (!result)
    4040            CRASH();
    41         return resultImpl.release();
     41        return result;
    4242    }
    4343
    … …  
    9797    void writeTo(LChar* destination) { m_buffer.writeTo(destination); }
    9898    void writeTo(UChar* destination) { m_buffer.writeTo(destination); }
     99
     100    String toString() const { return m_buffer; }
    99101
    100102private:

  • trunk/Source/WTF/wtf/text/StringView.h

    r185899 r190882  
    498498    void writeTo(UChar* destination) { m_string.getCharactersWithUpconvert(destination); }
    499499
     500    String toString() const { return m_string.toString(); }
     501
    500502private:
    501503    StringView m_string;
Note: See TracChangeset for help on using the changeset viewer.