Changeset 289359 in webkit


Ignore:
Timestamp:
Feb 7, 2022 9:47:11 PM (5 months ago)
Author:
ysuzuki@apple.com
Message:

[JSC] Convert JSString's non-atomic WTF::String to atomic string while concurrent compilers / heap threads run
​https://bugs.webkit.org/show_bug.cgi?id=236262

Reviewed by Saam Barati.

Inspired from r289177. This patch introduces a new protocol which allows us to replace JSString's underlying non-atomic String
to atomic String if we once call toIdentifier / toAtomString.

We had a problem that,

  1. We have a JSString, which has a "test" WTF::String.
  2. We already have "test" atomic string in the table.
  3. Then, when we call JSString::toIdentifier, we know that there is an atomic "test" string, but we cannot replace the current JSString's WTF::String because it can be accessed concurrently from concurrent compilers and GC heap helpers.
  4. Thus, JSString keeps non atomic "test" WTF::String.

But this means that we need to lookup atom string table every time we would like to get an atom string from this JSString.

So, in this patch, we introduce a new protocol, which allows swapping existing WTF::String with an atom string.

When we found that JSString has a WTF::String and we already have atom string in the table with the same content (when calling
toIdentifier / toAtomString), we attempt to replace JSString's WTF::String with the atom string, but *keep the old string in JSC::Heap's
vector called m_possiblyAccessedStringsFromConcurrentThreads. Then, we can keep these strings alive until next GC ends. This ensures that
all concurrent compilers / heap helpers can keep accessing to the old strings. And then, in the GC finalize, we clear this vector since
resumed concurrent compilers and GC heap helpers will not touch these old strings in the next GC cycle. Only case we have a problem is
that we keep having StringImpl* of the old string after GC safepoint in the concurrent compiler, and the only use of that is
DFG::Graph::m_copiedStrings. So, I changed the code not to keep old StringImpl* in DFG::Graph::m_copiedStrings. Also, note that we do
this only when we convert non-atom string to atom string so all UniquedStringImpl* from JSString* (it is atom ones) does not matter since
they are already atom one: they will not be replaced.

This does not increase memory usage, rather, improve memory usage since this kept string was anyway held by the wrapper's JSString at least
until the next GC run. And we clear m_possiblyAccessedStringsFromConcurrentThreads in the next GC run, so we can shrink memory.

It improves Speedometer2 by 0.2%.

----------------------------------------------------------------------------------------------------------------------------------
| subtest | ms | ms | b / a | pValue (significance using False Discovery Rate) |
----------------------------------------------------------------------------------------------------------------------------------
| Elm-TodoMVC |106.193333 |105.690000 |0.995260 | 0.050074 |
| VueJS-TodoMVC |21.671667 |21.741667 |1.003230 | 0.715305 |
| EmberJS-TodoMVC |113.146667 |110.871667 |0.979893 | 0.000000 (significant) |
| BackboneJS-TodoMVC |42.481667 |42.346667 |0.996822 | 0.358040 |
| Preact-TodoMVC |15.796667 |16.016667 |1.013927 | 0.226011 |
| AngularJS-TodoMVC |117.568333 |117.345000 |0.998100 | 0.543369 |
| Vanilla-ES2015-TodoMVC |58.348333 |57.905000 |0.992402 | 0.000381 (significant) |
| Inferno-TodoMVC |54.656667 |54.946667 |1.005306 | 0.254310 |
| Flight-TodoMVC |61.106667 |61.141667 |1.000573 | 0.880780 |
| Angular2-TypeScript-TodoMVC |37.030000 |37.065000 |1.000945 | 0.918550 |
| VanillaJS-TodoMVC |47.741667 |47.911667 |1.003561 | 0.497675 |
| jQuery-TodoMVC |205.251667 |203.903333 |0.993431 | 0.000420 (significant) |
| EmberJS-Debug-TodoMVC |312.448333 |308.848333 |0.988478 | 0.000020 (significant) |
| React-TodoMVC |78.381667 |78.268333 |0.998554 | 0.654647 |
| React-Redux-TodoMVC |131.246667 |131.626667 |1.002895 | 0.138912 |
| Vanilla-ES2015-Babel-Webpack-TodoMVC |57.860000 |57.533333 |0.994354 | 0.156536 |
----------------------------------------------------------------------------------------------------------------------------------
a mean = 290.61106
b mean = 291.21768
pValue = 0.1419936818
(Bigger means are better.)
1.002 times better
Results ARE NOT significant

  • bytecompiler/BytecodeGenerator.cpp:

(JSC::prepareJumpTableForStringSwitch):

  • dfg/DFGByteCodeParser.cpp:

(JSC::DFG::ByteCodeParser::parseBlock):

  • dfg/DFGGraph.h:
  • dfg/DFGLazyJSValue.cpp:

(JSC::DFG::CrossThreadStringTranslator::hash):
(JSC::DFG::CrossThreadStringTranslator::equal):
(JSC::DFG::CrossThreadStringTranslator::translate):
(JSC::DFG::LazyJSValue::tryGetString const):

  • dfg/DFGLazyJSValue.h:

(JSC::DFG::LazyJSValue::knownStringImpl):

  • heap/Heap.cpp:

(JSC::Heap::finalize):

  • heap/Heap.h:

(JSC::Heap::appendPossiblyAccessedStringFromConcurrentThreads):

  • runtime/JSString.h:

(JSC::JSString::swapToAtomString const):
(JSC::JSString::toIdentifier const):
(JSC::JSString::toAtomString const):

Location:
trunk/Source/JavaScriptCore
Files:
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r289354 r289359  
     12022-02-07  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] Convert JSString's non-atomic WTF::String to atomic string while concurrent compilers / heap threads run
     4        https://bugs.webkit.org/show_bug.cgi?id=236262
     5
     6        Reviewed by Saam Barati.
     7
     8        Inspired from r289177. This patch introduces a new protocol which allows us to replace JSString's underlying non-atomic String
     9        to atomic String if we once call toIdentifier / toAtomString.
     10
     11        We had a problem that,
     12
     13        1. We have a JSString, which has a "test" WTF::String.
     14        2. We already have "test" atomic string in the table.
     15        3. Then, when we call JSString::toIdentifier, we know that there is an atomic "test" string, but we cannot replace the current JSString's
     16           WTF::String because it can be accessed concurrently from concurrent compilers and GC heap helpers.
     17        4. Thus, JSString keeps non atomic "test" WTF::String.
     18
     19        But this means that we need to lookup atom string table every time we would like to get an atom string from this JSString.
     20
     21        So, in this patch, we introduce a new protocol, which allows swapping existing WTF::String with an atom string.
     22
     23        When we found that JSString has a WTF::String and we already have atom string in the table with the same content (when calling
     24        toIdentifier / toAtomString), we attempt to replace JSString's WTF::String with the atom string, but *keep the old string in JSC::Heap's
     25        vector called m_possiblyAccessedStringsFromConcurrentThreads. Then, we can keep these strings alive until next GC ends. This ensures that
     26        all concurrent compilers / heap helpers can keep accessing to the old strings. And then, in the GC finalize, we clear this vector since
     27        resumed concurrent compilers and GC heap helpers will not touch these old strings in the next GC cycle. Only case we have a problem is
     28        that we keep having StringImpl* of the old string after GC safepoint in the concurrent compiler, and the only use of that is
     29        DFG::Graph::m_copiedStrings. So, I changed the code not to keep old StringImpl* in DFG::Graph::m_copiedStrings. Also, note that we do
     30        this only when we convert non-atom string to atom string so all UniquedStringImpl* from JSString* (it is atom ones) does not matter since
     31        they are already atom one: they will not be replaced.
     32
     33        This does not increase memory usage, rather, improve memory usage since this kept string was anyway held by the wrapper's JSString at least
     34        until the next GC run. And we clear m_possiblyAccessedStringsFromConcurrentThreads in the next GC run, so we can shrink memory.
     35
     36        It improves Speedometer2 by 0.2%.
     37
     38            ----------------------------------------------------------------------------------------------------------------------------------
     39            |               subtest                |     ms      |     ms      |  b / a   | pValue (significance using False Discovery Rate) |
     40            ----------------------------------------------------------------------------------------------------------------------------------
     41            | Elm-TodoMVC                          |106.193333   |105.690000   |0.995260  | 0.050074                                         |
     42            | VueJS-TodoMVC                        |21.671667    |21.741667    |1.003230  | 0.715305                                         |
     43            | EmberJS-TodoMVC                      |113.146667   |110.871667   |0.979893  | 0.000000 (significant)                           |
     44            | BackboneJS-TodoMVC                   |42.481667    |42.346667    |0.996822  | 0.358040                                         |
     45            | Preact-TodoMVC                       |15.796667    |16.016667    |1.013927  | 0.226011                                         |
     46            | AngularJS-TodoMVC                    |117.568333   |117.345000   |0.998100  | 0.543369                                         |
     47            | Vanilla-ES2015-TodoMVC               |58.348333    |57.905000    |0.992402  | 0.000381 (significant)                           |
     48            | Inferno-TodoMVC                      |54.656667    |54.946667    |1.005306  | 0.254310                                         |
     49            | Flight-TodoMVC                       |61.106667    |61.141667    |1.000573  | 0.880780                                         |
     50            | Angular2-TypeScript-TodoMVC          |37.030000    |37.065000    |1.000945  | 0.918550                                         |
     51            | VanillaJS-TodoMVC                    |47.741667    |47.911667    |1.003561  | 0.497675                                         |
     52            | jQuery-TodoMVC                       |205.251667   |203.903333   |0.993431  | 0.000420 (significant)                           |
     53            | EmberJS-Debug-TodoMVC                |312.448333   |308.848333   |0.988478  | 0.000020 (significant)                           |
     54            | React-TodoMVC                        |78.381667    |78.268333    |0.998554  | 0.654647                                         |
     55            | React-Redux-TodoMVC                  |131.246667   |131.626667   |1.002895  | 0.138912                                         |
     56            | Vanilla-ES2015-Babel-Webpack-TodoMVC |57.860000    |57.533333    |0.994354  | 0.156536                                         |
     57            ----------------------------------------------------------------------------------------------------------------------------------
     58            a mean = 290.61106
     59            b mean = 291.21768
     60            pValue = 0.1419936818
     61            (Bigger means are better.)
     62            1.002 times better
     63            Results ARE NOT significant
     64
     65        * bytecompiler/BytecodeGenerator.cpp:
     66        (JSC::prepareJumpTableForStringSwitch):
     67        * dfg/DFGByteCodeParser.cpp:
     68        (JSC::DFG::ByteCodeParser::parseBlock):
     69        * dfg/DFGGraph.h:
     70        * dfg/DFGLazyJSValue.cpp:
     71        (JSC::DFG::CrossThreadStringTranslator::hash):
     72        (JSC::DFG::CrossThreadStringTranslator::equal):
     73        (JSC::DFG::CrossThreadStringTranslator::translate):
     74        (JSC::DFG::LazyJSValue::tryGetString const):
     75        * dfg/DFGLazyJSValue.h:
     76        (JSC::DFG::LazyJSValue::knownStringImpl):
     77        * heap/Heap.cpp:
     78        (JSC::Heap::finalize):
     79        * heap/Heap.h:
     80        (JSC::Heap::appendPossiblyAccessedStringFromConcurrentThreads):
     81        * runtime/JSString.h:
     82        (JSC::JSString::swapToAtomString const):
     83        (JSC::JSString::toIdentifier const):
     84        (JSC::JSString::toAtomString const):
     85
    1862022-02-07  Saam Barati  <sbarati@apple.com>
    287

  • trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

    r284690 r289359  
    41824182       
    41834183        ASSERT(nodes[i]->isString());
    4184         StringImpl* clause = static_cast<StringNode*>(nodes[i])->value().impl();
     4184        UniquedStringImpl* clause = static_cast<StringNode*>(nodes[i])->value().impl();
     4185        ASSERT(clause->isAtom());
    41854186        auto result = jumpTable.m_offsetTable.add(clause, UnlinkedStringJumpTable::OffsetLocation { labels[i]->bind(switchAddress), 0 });
    41864187        if (result.isNewEntry)

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

    r288815 r289359  
    70087008                if (target == data.fallThrough.bytecodeIndex())
    70097009                    continue;
     7010                ASSERT(entry.key.get()->isAtom());
    70107011                data.cases.append(
    7011                     SwitchCase::withBytecodeIndex(LazyJSValue::knownStringImpl(entry.key.get()), target));
     7012                    SwitchCase::withBytecodeIndex(LazyJSValue::knownStringImpl(static_cast<AtomStringImpl*>(entry.key.get())), target));
    70127013            }
    70137014            addToGraph(Switch, OpInfo(&data), get(bytecode.m_scrutinee));

  • trunk/Source/JavaScriptCore/dfg/DFGGraph.h

    r288807 r289359  
    11771177
    11781178    HashSet<String> m_localStrings;
    1179     HashMap<const StringImpl*, String> m_copiedStrings;
     1179    HashSet<String> m_copiedStrings;
    11801180
    11811181#if USE(JSVALUE32_64)

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

    r278030 r289359  
    108108}
    109109
     110struct CrossThreadStringTranslator {
     111    static unsigned hash(const StringImpl* impl)
     112    {
     113        return impl->concurrentHash();
     114    }
     115
     116    static bool equal(const String& string, const StringImpl* impl)
     117    {
     118        return WTF::equal(string.impl(), impl);
     119    }
     120
     121    static void translate(String& location, const StringImpl* impl, unsigned)
     122    {
     123        location = impl->isolatedCopy();
     124    }
     125};
     126
    110127String LazyJSValue::tryGetString(Graph& graph) const
    111128{
    … …  
    124141                return String();
    125142           
    126             auto result = graph.m_copiedStrings.add(string, String());
    127             if (result.isNewEntry)
    128                 result.iterator->value = string->isolatedCopy();
    129             return result.iterator->value;
     143            return *graph.m_copiedStrings.add<CrossThreadStringTranslator>(string).iterator;
    130144        }
    131145       

  • trunk/Source/JavaScriptCore/dfg/DFGLazyJSValue.h

    r260323 r289359  
    6767    }
    6868   
    69     static LazyJSValue knownStringImpl(StringImpl* string)
     69    static LazyJSValue knownStringImpl(AtomStringImpl* string)
    7070    {
    7171        LazyJSValue result;
    … …  
    101101    }
    102102
    103     const StringImpl* tryGetStringImpl(VM&) const;
    104    
    105103    String tryGetString(Graph&) const;
    106104   
    … …  
    121119   
    122120private:
     121    const StringImpl* tryGetStringImpl(VM&) const;
     122   
    123123    union {
    124124        FrozenValue* value;

  • trunk/Source/JavaScriptCore/heap/Heap.cpp

    r288815 r289359  
    21592159    if (m_lastCollectionScope && m_lastCollectionScope.value() == CollectionScope::Full)
    21602160        vm().jsonAtomStringCache.clear();
     2161
     2162    m_possiblyAccessedStringsFromConcurrentThreads.clear();
    21612163
    21622164    immutableButterflyToStringCache.clear();

  • trunk/Source/JavaScriptCore/heap/Heap.h

    r288815 r289359  
    390390    bool isMarkingForGCVerifier() const { return m_isMarkingForGCVerifier; }
    391391
     392    void appendPossiblyAccessedStringFromConcurrentThreads(String&& string)
     393    {
     394        m_possiblyAccessedStringsFromConcurrentThreads.append(WTFMove(string));
     395    }
     396
    392397private:
    393398    friend class AllocatingScope;
    … …  
    639644    Vector<WeakBlock*> m_logicallyEmptyWeakBlocks;
    640645    size_t m_indexOfNextLogicallyEmptyWeakBlockToSweep { WTF::notFound };
     646
     647    Vector<String> m_possiblyAccessedStringsFromConcurrentThreads;
    641648   
    642649    RefPtr<FullGCActivityCallback> m_fullActivityCallback;

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

    r289177 r289359  
    246246
    247247    StringView unsafeView(JSGlobalObject*) const;
     248
     249    void swapToAtomString(VM&, RefPtr<AtomStringImpl>&&) const;
    248250
    249251    friend JSString* jsString(VM&, const String&);
    … …  
    770772}
    771773
     774ALWAYS_INLINE void JSString::swapToAtomString(VM& vm, RefPtr<AtomStringImpl>&& atom) const
     775{
     776    // We replace currently held string with new AtomString. But the old string can be accessed from concurrent compilers and GC threads at any time.
     777    // So, we keep the old string alive by appending it to Heap::m_possiblyAccessedStringsFromConcurrentThreads. And GC clears that list when GC finishes.
     778    // This is OK since (1) when finishing GC concurrent compiler threads and GC threads are stopped, and (2) AtomString is already held in the atom table,
     779    // and we anyway keep this old string until this JSString* is GC-ed. So it does not increase any memory pressure, we release at the same timing.
     780    ASSERT(!isCompilationThread() && !Thread::mayBeGCThread());
     781    String target(WTFMove(atom));
     782    WTF::storeStoreFence(); // Ensure AtomStringImpl's string is fully initialized when it is exposed to concurrent threads.
     783    valueInternal().swap(target);
     784    vm.heap.appendPossiblyAccessedStringFromConcurrentThreads(WTFMove(target));
     785}
     786
    772787ALWAYS_INLINE Identifier JSString::toIdentifier(JSGlobalObject* globalObject) const
    773788{
    … …  
    783798        vm.lastAtomizedIdentifierAtomStringImpl = AtomStringImpl::add(valueInternal().impl()).releaseNonNull();
    784799    }
     800    // It is possible that AtomStringImpl::add converts existing valueInternal()'s StringImpl to AtomicStringImpl,
     801    // thus we need to recheck atomicity status here.
     802    if (!valueInternal().impl()->isAtom())
     803        swapToAtomString(vm, RefPtr { vm.lastAtomizedIdentifierAtomStringImpl.ptr() });
    785804    return Identifier::fromString(vm, Ref { vm.lastAtomizedIdentifierAtomStringImpl });
    786805}
    … …  
    792811    if (isRope())
    793812        return static_cast<const JSRopeString*>(this)->resolveRopeToAtomString(globalObject);
    794     return AtomString(valueInternal());
     813    AtomString atom(valueInternal());
     814    // It is possible that AtomString constructor converts existing valueInternal()'s StringImpl to AtomicStringImpl,
     815    // thus we need to recheck atomicity status here.
     816    if (!valueInternal().impl()->isAtom())
     817        swapToAtomString(getVM(globalObject), RefPtr { atom.impl() });
     818    return atom;
    795819}
    796820
Note: See TracChangeset for help on using the changeset viewer.