- Revision
- 289359
- Author
- [email protected]
- Date
- 2022-02-07 21:47:11 -0800 (Mon, 07 Feb 2022)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (289358 => 289359)
--- trunk/Source/_javascript_Core/ChangeLog 2022-02-08 04:59:58 UTC (rev 289358)
+++ trunk/Source/_javascript_Core/ChangeLog 2022-02-08 05:47:11 UTC (rev 289359)
@@ -1,3 +1,88 @@
+2022-02-07 Yusuke Suzuki <[email protected]>
+
+ [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):
+
2022-02-07 Saam Barati <[email protected]>
Wasm crash on https://copy.sh/v86/?profile=""
Modified: trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp (289358 => 289359)
--- trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp 2022-02-08 04:59:58 UTC (rev 289358)
+++ trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp 2022-02-08 05:47:11 UTC (rev 289359)
@@ -4181,7 +4181,8 @@
ASSERT(!labels[i]->isForward());
ASSERT(nodes[i]->isString());
- StringImpl* clause = static_cast<StringNode*>(nodes[i])->value().impl();
+ UniquedStringImpl* clause = static_cast<StringNode*>(nodes[i])->value().impl();
+ ASSERT(clause->isAtom());
auto result = jumpTable.m_offsetTable.add(clause, UnlinkedStringJumpTable::OffsetLocation { labels[i]->bind(switchAddress), 0 });
if (result.isNewEntry)
result.iterator->value.m_indexInTable = jumpTable.m_offsetTable.size() - 1;
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (289358 => 289359)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2022-02-08 04:59:58 UTC (rev 289358)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2022-02-08 05:47:11 UTC (rev 289359)
@@ -7007,8 +7007,9 @@
unsigned target = m_currentIndex.offset() + entry.value.m_branchOffset;
if (target == data.fallThrough.bytecodeIndex())
continue;
+ ASSERT(entry.key.get()->isAtom());
data.cases.append(
- SwitchCase::withBytecodeIndex(LazyJSValue::knownStringImpl(entry.key.get()), target));
+ SwitchCase::withBytecodeIndex(LazyJSValue::knownStringImpl(static_cast<AtomStringImpl*>(entry.key.get())), target));
}
addToGraph(Switch, OpInfo(&data), get(bytecode.m_scrutinee));
flushIfTerminal(data);
Modified: trunk/Source/_javascript_Core/dfg/DFGGraph.h (289358 => 289359)
--- trunk/Source/_javascript_Core/dfg/DFGGraph.h 2022-02-08 04:59:58 UTC (rev 289358)
+++ trunk/Source/_javascript_Core/dfg/DFGGraph.h 2022-02-08 05:47:11 UTC (rev 289359)
@@ -1176,7 +1176,7 @@
Vector<CatchEntrypointData> m_catchEntrypoints;
HashSet<String> m_localStrings;
- HashMap<const StringImpl*, String> m_copiedStrings;
+ HashSet<String> m_copiedStrings;
#if USE(JSVALUE32_64)
HashMap<GenericHashKey<int64_t>, double*> m_doubleConstantsMap;
Modified: trunk/Source/_javascript_Core/dfg/DFGLazyJSValue.cpp (289358 => 289359)
--- trunk/Source/_javascript_Core/dfg/DFGLazyJSValue.cpp 2022-02-08 04:59:58 UTC (rev 289358)
+++ trunk/Source/_javascript_Core/dfg/DFGLazyJSValue.cpp 2022-02-08 05:47:11 UTC (rev 289359)
@@ -107,6 +107,23 @@
return nullptr;
}
+struct CrossThreadStringTranslator {
+ static unsigned hash(const StringImpl* impl)
+ {
+ return impl->concurrentHash();
+ }
+
+ static bool equal(const String& string, const StringImpl* impl)
+ {
+ return WTF::equal(string.impl(), impl);
+ }
+
+ static void translate(String& location, const StringImpl* impl, unsigned)
+ {
+ location = impl->isolatedCopy();
+ }
+};
+
String LazyJSValue::tryGetString(Graph& graph) const
{
switch (m_kind) {
@@ -123,10 +140,7 @@
if (string->length() > ginormousStringLength)
return String();
- auto result = graph.m_copiedStrings.add(string, String());
- if (result.isNewEntry)
- result.iterator->value = string->isolatedCopy();
- return result.iterator->value;
+ return *graph.m_copiedStrings.add<CrossThreadStringTranslator>(string).iterator;
}
return String();
Modified: trunk/Source/_javascript_Core/dfg/DFGLazyJSValue.h (289358 => 289359)
--- trunk/Source/_javascript_Core/dfg/DFGLazyJSValue.h 2022-02-08 04:59:58 UTC (rev 289358)
+++ trunk/Source/_javascript_Core/dfg/DFGLazyJSValue.h 2022-02-08 05:47:11 UTC (rev 289359)
@@ -66,7 +66,7 @@
return result;
}
- static LazyJSValue knownStringImpl(StringImpl* string)
+ static LazyJSValue knownStringImpl(AtomStringImpl* string)
{
LazyJSValue result;
result.m_kind = KnownStringImpl;
@@ -100,8 +100,6 @@
return u.character;
}
- const StringImpl* tryGetStringImpl(VM&) const;
-
String tryGetString(Graph&) const;
StringImpl* stringImpl() const
@@ -120,6 +118,8 @@
void dumpInContext(PrintStream&, DumpContext*) const;
private:
+ const StringImpl* tryGetStringImpl(VM&) const;
+
union {
FrozenValue* value;
UChar character;
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (289358 => 289359)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2022-02-08 04:59:58 UTC (rev 289358)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2022-02-08 05:47:11 UTC (rev 289359)
@@ -2159,6 +2159,8 @@
if (m_lastCollectionScope && m_lastCollectionScope.value() == CollectionScope::Full)
vm().jsonAtomStringCache.clear();
+ m_possiblyAccessedStringsFromConcurrentThreads.clear();
+
immutableButterflyToStringCache.clear();
for (const HeapFinalizerCallback& callback : m_heapFinalizerCallbacks)
Modified: trunk/Source/_javascript_Core/heap/Heap.h (289358 => 289359)
--- trunk/Source/_javascript_Core/heap/Heap.h 2022-02-08 04:59:58 UTC (rev 289358)
+++ trunk/Source/_javascript_Core/heap/Heap.h 2022-02-08 05:47:11 UTC (rev 289359)
@@ -389,6 +389,11 @@
bool isMarkingForGCVerifier() const { return m_isMarkingForGCVerifier; }
+ void appendPossiblyAccessedStringFromConcurrentThreads(String&& string)
+ {
+ m_possiblyAccessedStringsFromConcurrentThreads.append(WTFMove(string));
+ }
+
private:
friend class AllocatingScope;
friend class CodeBlock;
@@ -638,6 +643,8 @@
Vector<WeakBlock*> m_logicallyEmptyWeakBlocks;
size_t m_indexOfNextLogicallyEmptyWeakBlockToSweep { WTF::notFound };
+
+ Vector<String> m_possiblyAccessedStringsFromConcurrentThreads;
RefPtr<FullGCActivityCallback> m_fullActivityCallback;
RefPtr<GCActivityCallback> m_edenActivityCallback;
Modified: trunk/Source/_javascript_Core/runtime/JSString.h (289358 => 289359)
--- trunk/Source/_javascript_Core/runtime/JSString.h 2022-02-08 04:59:58 UTC (rev 289358)
+++ trunk/Source/_javascript_Core/runtime/JSString.h 2022-02-08 05:47:11 UTC (rev 289359)
@@ -246,6 +246,8 @@
StringView unsafeView(JSGlobalObject*) const;
+ void swapToAtomString(VM&, RefPtr<AtomStringImpl>&&) const;
+
friend JSString* jsString(VM&, const String&);
friend JSString* jsString(JSGlobalObject*, JSString*, JSString*);
friend JSString* jsString(JSGlobalObject*, const String&, JSString*);
@@ -769,6 +771,19 @@
return Identifier::fromString(vm, atomString);
}
+ALWAYS_INLINE void JSString::swapToAtomString(VM& vm, RefPtr<AtomStringImpl>&& atom) const
+{
+ // We replace currently held string with new AtomString. But the old string can be accessed from concurrent compilers and GC threads at any time.
+ // So, we keep the old string alive by appending it to Heap::m_possiblyAccessedStringsFromConcurrentThreads. And GC clears that list when GC finishes.
+ // 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,
+ // 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.
+ ASSERT(!isCompilationThread() && !Thread::mayBeGCThread());
+ String target(WTFMove(atom));
+ WTF::storeStoreFence(); // Ensure AtomStringImpl's string is fully initialized when it is exposed to concurrent threads.
+ valueInternal().swap(target);
+ vm.heap.appendPossiblyAccessedStringFromConcurrentThreads(WTFMove(target));
+}
+
ALWAYS_INLINE Identifier JSString::toIdentifier(JSGlobalObject* globalObject) const
{
if constexpr (validateDFGDoesGC)
@@ -782,6 +797,10 @@
vm.lastAtomizedIdentifierStringImpl = *valueInternal().impl();
vm.lastAtomizedIdentifierAtomStringImpl = AtomStringImpl::add(valueInternal().impl()).releaseNonNull();
}
+ // It is possible that AtomStringImpl::add converts existing valueInternal()'s StringImpl to AtomicStringImpl,
+ // thus we need to recheck atomicity status here.
+ if (!valueInternal().impl()->isAtom())
+ swapToAtomString(vm, RefPtr { vm.lastAtomizedIdentifierAtomStringImpl.ptr() });
return Identifier::fromString(vm, Ref { vm.lastAtomizedIdentifierAtomStringImpl });
}
@@ -791,7 +810,12 @@
vm().verifyCanGC();
if (isRope())
return static_cast<const JSRopeString*>(this)->resolveRopeToAtomString(globalObject);
- return AtomString(valueInternal());
+ AtomString atom(valueInternal());
+ // It is possible that AtomString constructor converts existing valueInternal()'s StringImpl to AtomicStringImpl,
+ // thus we need to recheck atomicity status here.
+ if (!valueInternal().impl()->isAtom())
+ swapToAtomString(getVM(globalObject), RefPtr { atom.impl() });
+ return atom;
}
ALWAYS_INLINE RefPtr<AtomStringImpl> JSString::toExistingAtomString(JSGlobalObject* globalObject) const
