Changeset 241493 in webkit


Ignore:
Timestamp:
Feb 13, 2019 7:01:25 PM (5 years ago)
Author:
ysuzuki@apple.com
Message:

We should only make rope strings when concatenating strings long enough.
​https://bugs.webkit.org/show_bug.cgi?id=194465

Reviewed by Mark Lam.

Source/JavaScriptCore:

This patch stops us from allocating a rope string if the resulting
rope would be smaller than the size of the JSRopeString object we
would need to allocate.

This patch also adds paths so that we don't unnecessarily allocate
JSString cells for primitives we are going to concatenate with a
string anyway.

The important change from the previous one is that we do not apply
the above rule to JSRopeStrings generated by JSStrings. If we convert
it to JSString, comparison of memory consumption becomes the following,
because JSRopeString does not have StringImpl until it is resolved.

sizeof(JSRopeString) v.s. sizeof(JSString) + sizeof(StringImpl) + content

Since sizeof(JSString) + sizeof(StringImpl) is larger than sizeof(JSRopeString),
resolving eagerly increases memory footprint. The point is that we need to
account newly created JSString and JSRopeString from the operands. This is the
reason why this patch adds different thresholds for each jsString functions.

This patch also avoids concatenation for ropes conservatively. Many ropes are
temporary cells. So we do not resolve eagerly if one of operands is already a
rope.

In CLI execution, this change is performance neutral in JetStream2 (run 6 times, 1 for warming up and average in latter 5.).

Before: 159.3778
After: 160.72340000000003

  • dfg/DFGOperations.cpp:
  • runtime/CommonSlowPaths.cpp:

(JSC::SLOW_PATH_DECL):

  • runtime/JSString.h:

(JSC::JSString::isRope const):

  • runtime/Operations.cpp:

(JSC::jsAddSlowCase):

  • runtime/Operations.h:

(JSC::jsString):
(JSC::jsAddNonNumber):
(JSC::jsAdd):

Source/WTF:

  • wtf/text/StringImpl.h:

(WTF::StringImpl::headerSize):

Location:
trunk/Source
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r241475 r241493  
     12019-02-13  Keith Miller  <keith_miller@apple.com> and Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        We should only make rope strings when concatenating strings long enough.
     4        https://bugs.webkit.org/show_bug.cgi?id=194465
     5
     6        Reviewed by Mark Lam.
     7
     8        This patch stops us from allocating a rope string if the resulting
     9        rope would be smaller than the size of the JSRopeString object we
     10        would need to allocate.
     11
     12        This patch also adds paths so that we don't unnecessarily allocate
     13        JSString cells for primitives we are going to concatenate with a
     14        string anyway.
     15
     16        The important change from the previous one is that we do not apply
     17        the above rule to JSRopeStrings generated by JSStrings. If we convert
     18        it to JSString, comparison of memory consumption becomes the following,
     19        because JSRopeString does not have StringImpl until it is resolved.
     20
     21            sizeof(JSRopeString) v.s. sizeof(JSString) + sizeof(StringImpl) + content
     22
     23        Since sizeof(JSString) + sizeof(StringImpl) is larger than sizeof(JSRopeString),
     24        resolving eagerly increases memory footprint. The point is that we need to
     25        account newly created JSString and JSRopeString from the operands. This is the
     26        reason why this patch adds different thresholds for each jsString functions.
     27
     28        This patch also avoids concatenation for ropes conservatively. Many ropes are
     29        temporary cells. So we do not resolve eagerly if one of operands is already a
     30        rope.
     31
     32        In CLI execution, this change is performance neutral in JetStream2 (run 6 times, 1 for warming up and average in latter 5.).
     33
     34            Before: 159.3778
     35            After:  160.72340000000003
     36
     37        * dfg/DFGOperations.cpp:
     38        * runtime/CommonSlowPaths.cpp:
     39        (JSC::SLOW_PATH_DECL):
     40        * runtime/JSString.h:
     41        (JSC::JSString::isRope const):
     42        * runtime/Operations.cpp:
     43        (JSC::jsAddSlowCase):
     44        * runtime/Operations.h:
     45        (JSC::jsString):
     46        (JSC::jsAddNonNumber):
     47        (JSC::jsAdd):
     48
    1492019-02-13  Saam Barati  <sbarati@apple.com>
    250

  • trunk/Source/JavaScriptCore/dfg/DFGOperations.cpp

    r241255 r241493  
    504504    JSValue op2 = JSValue::decode(encodedOp2);
    505505   
    506     ASSERT(!op1.isNumber() || !op2.isNumber());
    507    
    508     if (op1.isString() && !op2.isObject())
    509         return JSValue::encode(jsString(exec, asString(op1), op2.toString(exec)));
    510 
    511     return JSValue::encode(jsAddSlowCase(exec, op1, op2));
     506    return JSValue::encode(jsAddNonNumber(exec, op1, op2));
    512507}
    513508

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

    r241255 r241493  
    518518    JSValue v1 = GET_C(bytecode.m_lhs).jsValue();
    519519    JSValue v2 = GET_C(bytecode.m_rhs).jsValue();
    520     JSValue result;
    521520
    522521    ArithProfile& arithProfile = *exec->codeBlock()->arithProfileForPC(pc);
    523522    arithProfile.observeLHSAndRHS(v1, v2);
    524523
    525     if (v1.isString() && !v2.isObject()) {
    526         JSString* v2String = v2.toString(exec);
    527         if (LIKELY(!throwScope.exception()))
    528             result = jsString(exec, asString(v1), v2String);
    529     } else if (v1.isNumber() && v2.isNumber())
    530         result = jsNumber(v1.asNumber() + v2.asNumber());
    531     else
    532         result = jsAddSlowCase(exec, v1, v2);
     524    JSValue result = jsAdd(exec, v1, v2);
    533525
    534526    RETURN_WITH_PROFILING(result, {

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

    r241255 r241493  
    197197    };
    198198
     199    bool isRope() const { return m_value.isNull(); }
     200
    199201protected:
    200202    friend class JSValue;
    201203
    202204    JS_EXPORT_PRIVATE bool equalSlowCase(ExecState*, JSString* other) const;
    203     bool isRope() const { return m_value.isNull(); }
    204205    bool isSubstring() const;
    205206    bool is8Bit() const { return m_flags & Is8Bit; }
    … …  
    487488
    488489    friend JSString* jsString(ExecState*, JSString*, JSString*);
     490    friend JSString* jsString(ExecState*, const String&, JSString*);
     491    friend JSString* jsString(ExecState*, JSString*, const String&);
    489492    friend JSString* jsString(ExecState*, JSString*, JSString*, JSString*);
     493    friend JSString* jsString(ExecState*, const String&, const String&);
    490494    friend JSString* jsString(ExecState*, const String&, const String&, const String&);
    491495};

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

    r241255 r241493  
    5353
    5454    if (p1.isString()) {
    55         JSString* p2String = p2.toString(callFrame);
     55        if (p2.isCell()) {
     56            JSString* p2String = p2.toString(callFrame);
     57            RETURN_IF_EXCEPTION(scope, { });
     58            RELEASE_AND_RETURN(scope, jsString(callFrame, asString(p1), p2String));
     59        }
     60        String p2String = p2.toWTFString(callFrame);
    5661        RETURN_IF_EXCEPTION(scope, { });
    5762        RELEASE_AND_RETURN(scope, jsString(callFrame, asString(p1), p2String));
    … …  
    5964
    6065    if (p2.isString()) {
    61         JSString* p1String = p1.toString(callFrame);
     66        if (p1.isCell()) {
     67            JSString* p1String = p1.toString(callFrame);
     68            RETURN_IF_EXCEPTION(scope, { });
     69            RELEASE_AND_RETURN(scope, jsString(callFrame, p1String, asString(p2)));
     70        }
     71        String p1String = p1.toWTFString(callFrame);
    6272        RETURN_IF_EXCEPTION(scope, { });
    6373        RELEASE_AND_RETURN(scope, jsString(callFrame, p1String, asString(p2)));

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

    r241255 r241493  
    3838size_t normalizePrototypeChain(CallFrame*, JSCell*, bool& sawPolyProto);
    3939
     40ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, JSString* s2)
     41{
     42    VM& vm = exec->vm();
     43    auto scope = DECLARE_THROW_SCOPE(vm);
     44
     45    unsigned length1 = u1.length();
     46    if (!length1)
     47        return s2;
     48    unsigned length2 = s2->length();
     49    if (!length2)
     50        return jsString(&vm, u1);
     51    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
     52    if (sumOverflows<int32_t>(length1, length2)) {
     53        throwOutOfMemoryError(exec, scope);
     54        return nullptr;
     55    }
     56
     57    // (1) Cost of making JSString    : sizeof(JSString) (for new string) + sizeof(StringImpl header) + length1 + length2
     58    // (2) Cost of making JSRopeString: sizeof(JSString) (for u1) + sizeof(JSRopeString)
     59    // We do not account u1 cost in (2) since u1 may be shared StringImpl, and it may not introduce additional cost.
     60    // We conservatively consider the cost of u1. Currently, we are not considering about is8Bit() case because 16-bit
     61    // strings are relatively rare. But we can do that if we need to consider it.
     62    if (s2->isRope() || (StringImpl::headerSize<LChar>() + length1 + length2) >= sizeof(JSRopeString))
     63        return JSRopeString::create(vm, jsString(&vm, u1), s2);
     64
     65    ASSERT(!s2->isRope());
     66    const String& u2 = s2->value(exec);
     67    scope.assertNoException();
     68    String newString = tryMakeString(u1, u2);
     69    if (!newString) {
     70        throwOutOfMemoryError(exec, scope);
     71        return nullptr;
     72    }
     73    return JSString::create(vm, newString.releaseImpl().releaseNonNull());
     74}
     75
     76ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, const String& u2)
     77{
     78    VM& vm = exec->vm();
     79    auto scope = DECLARE_THROW_SCOPE(vm);
     80
     81    unsigned length1 = s1->length();
     82    if (!length1)
     83        return jsString(&vm, u2);
     84    unsigned length2 = u2.length();
     85    if (!length2)
     86        return s1;
     87    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
     88    if (sumOverflows<int32_t>(length1, length2)) {
     89        throwOutOfMemoryError(exec, scope);
     90        return nullptr;
     91    }
     92
     93    // (1) Cost of making JSString    : sizeof(JSString) (for new string) + sizeof(StringImpl header) + length1 + length2
     94    // (2) Cost of making JSRopeString: sizeof(JSString) (for u2) + sizeof(JSRopeString)
     95    if (s1->isRope() || (StringImpl::headerSize<LChar>() + length1 + length2) >= sizeof(JSRopeString))
     96        return JSRopeString::create(vm, s1, jsString(&vm, u2));
     97
     98    ASSERT(!s1->isRope());
     99    const String& u1 = s1->value(exec);
     100    scope.assertNoException();
     101    String newString = tryMakeString(u1, u2);
     102    if (!newString) {
     103        throwOutOfMemoryError(exec, scope);
     104        return nullptr;
     105    }
     106    return JSString::create(vm, newString.releaseImpl().releaseNonNull());
     107}
     108
    40109ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2)
    41110{
    … …  
    80149        return nullptr;
    81150    }
     151
    82152    return JSRopeString::create(vm, s1, s2, s3);
     153}
     154
     155ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String& u2)
     156{
     157    VM& vm = exec->vm();
     158    auto scope = DECLARE_THROW_SCOPE(vm);
     159
     160    unsigned length1 = u1.length();
     161    if (!length1)
     162        return jsString(&vm, u2);
     163    unsigned length2 = u2.length();
     164    if (!length2)
     165        return jsString(&vm, u1);
     166    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
     167    if (sumOverflows<int32_t>(length1, length2)) {
     168        throwOutOfMemoryError(exec, scope);
     169        return nullptr;
     170    }
     171
     172    // (1) Cost of making JSString    : sizeof(JSString) (for new string) + sizeof(StringImpl header) + length1 + length2
     173    // (2) Cost of making JSRopeString: sizeof(JSString) (for u1) + sizeof(JSString) (for u2) + sizeof(JSRopeString)
     174    if ((StringImpl::headerSize<LChar>() + length1 + length2) >= (sizeof(JSRopeString) + sizeof(JSString)))
     175        return JSRopeString::create(vm, jsString(&vm, u1), jsString(&vm, u2));
     176
     177    String newString = tryMakeString(u1, u2);
     178    if (!newString) {
     179        throwOutOfMemoryError(exec, scope);
     180        return nullptr;
     181    }
     182    return JSString::create(vm, newString.releaseImpl().releaseNonNull());
    83183}
    84184
    … …  
    96196
    97197    if (!length1)
    98         RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u2), jsString(vm, u3)));
     198        RELEASE_AND_RETURN(scope, jsString(exec, u2, u3));
    99199
    100200    if (!length2)
    101         RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u1), jsString(vm, u3)));
     201        RELEASE_AND_RETURN(scope, jsString(exec, u1, u3));
    102202
    103203    if (!length3)
    104         RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u1), jsString(vm, u2)));
     204        RELEASE_AND_RETURN(scope, jsString(exec, u1, u2));
    105205
    106206    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
    … …  
    110210    }
    111211
    112     return JSRopeString::create(*vm, jsString(vm, u1), jsString(vm, u2), jsString(vm, u3));
     212    // (1) Cost of making JSString    : sizeof(JSString) (for new string) + sizeof(StringImpl header) + length1 + length2 + length3
     213    // (2) Cost of making JSRopeString: sizeof(JSString) (for u1) + sizeof(JSString) (for u2) + sizeof(JSString) (for u3) + sizeof(JSRopeString)
     214    if ((StringImpl::headerSize<LChar>() + length1 + length2 + length3) >= (sizeof(JSRopeString) + sizeof(JSString) * 2))
     215        return JSRopeString::create(*vm, jsString(vm, u1), jsString(vm, u2), jsString(vm, u3));
     216
     217    String newString = tryMakeString(u1, u2, u3);
     218    if (!newString) {
     219        throwOutOfMemoryError(exec, scope);
     220        return nullptr;
     221    }
     222    return JSString::create(*vm, newString.releaseImpl().releaseNonNull());
    113223}
    114224
    … …  
    326436//    4000    Add case: 3 5
    327437
     438
     439ALWAYS_INLINE JSValue jsAddNonNumber(CallFrame* callFrame, JSValue v1, JSValue v2)
     440{
     441    VM& vm = callFrame->vm();
     442    auto scope = DECLARE_THROW_SCOPE(vm);
     443    ASSERT(!v1.isNumber() || !v2.isNumber());
     444
     445    if (LIKELY(v1.isString() && !v2.isObject())) {
     446        if (v2.isString())
     447            RELEASE_AND_RETURN(scope, jsString(callFrame, asString(v1), asString(v2)));
     448        String s2 = v2.toWTFString(callFrame);
     449        RETURN_IF_EXCEPTION(scope, { });
     450        RELEASE_AND_RETURN(scope, jsString(callFrame, asString(v1), s2));
     451    }
     452
     453    // All other cases are pretty uncommon
     454    RELEASE_AND_RETURN(scope, jsAddSlowCase(callFrame, v1, v2));
     455}
     456
    328457ALWAYS_INLINE JSValue jsAdd(CallFrame* callFrame, JSValue v1, JSValue v2)
    329458{
    … …  
    331460        return jsNumber(v1.asNumber() + v2.asNumber());
    332461       
    333     if (v1.isString() && !v2.isObject())
    334         return jsString(callFrame, asString(v1), v2.toString(callFrame));
    335 
    336     // All other cases are pretty uncommon
    337     return jsAddSlowCase(callFrame, v1, v2);
     462    return jsAddNonNumber(callFrame, v1, v2);
    338463}
    339464

  • trunk/Source/WTF/ChangeLog

    r241337 r241493  
     12019-02-13  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        We should only make rope strings when concatenating strings long enough.
     4        https://bugs.webkit.org/show_bug.cgi?id=194465
     5
     6        Reviewed by Mark Lam.
     7
     8        * wtf/text/StringImpl.h:
     9        (WTF::StringImpl::headerSize):
     10
    1112019-02-12  Tim Horton  <timothy_horton@apple.com>
    212

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

    r241242 r241493  
    486486
    487487    BufferOwnership bufferOwnership() const { return static_cast<BufferOwnership>(m_hashAndFlags & s_hashMaskBufferOwnership); }
     488
     489    template<typename T> static size_t headerSize() { return tailOffset<T>(); }
    488490   
    489491protected:
Note: See TracChangeset for help on using the changeset viewer.