Modified: trunk/Source/WTF/ChangeLog (210229 => 210230)
--- trunk/Source/WTF/ChangeLog 2017-01-03 01:57:40 UTC (rev 210229)
+++ trunk/Source/WTF/ChangeLog 2017-01-03 02:40:45 UTC (rev 210230)
@@ -1,5 +1,31 @@
2017-01-02 Yusuke Suzuki <[email protected]>
+ Leverage Substring to create new AtomicStringImpl for StaticStringImpl and SymbolImpl
+ https://bugs.webkit.org/show_bug.cgi?id=166636
+
+ Reviewed by Darin Adler.
+
+ Previously we always create the full atomic string if we need to create the same string
+ based on the given value. For example, when generating AtomicStringImpl from the SymbolImpl,
+ we need to create a new AtomicStringImpl since SymbolImpl never becomes `isAtomic() == true`.
+ But it is costly.
+
+ This patch leverages the substring system of StringImpl. Instead of allocating the completely
+ duplicate string, we create a substring StringImpl that shares the same content with the
+ base string.
+
+ * wtf/text/AtomicStringImpl.cpp:
+ (WTF::stringTable):
+ (WTF::addToStringTable):
+ (WTF::addSubstring):
+ (WTF::AtomicStringImpl::addSlowCase):
+ (WTF::AtomicStringImpl::remove):
+ (WTF::AtomicStringImpl::lookUpSlowCase):
+ * wtf/text/StringImpl.h:
+ (WTF::StringImpl::StaticStringImpl::operator StringImpl&):
+
+2017-01-02 Yusuke Suzuki <[email protected]>
+
Use StaticStringImpl instead of StaticASCIILiteral
https://bugs.webkit.org/show_bug.cgi?id=166586
Modified: trunk/Source/WTF/wtf/text/AtomicStringImpl.cpp (210229 => 210230)
--- trunk/Source/WTF/wtf/text/AtomicStringImpl.cpp 2017-01-03 01:57:40 UTC (rev 210229)
+++ trunk/Source/WTF/wtf/text/AtomicStringImpl.cpp 2017-01-03 02:40:45 UTC (rev 210230)
@@ -68,19 +68,18 @@
#endif // USE(WEB_THREAD)
-static ALWAYS_INLINE HashSet<StringImpl*>& stringTable()
+using StringTableImpl = HashSet<StringImpl*>;
+
+static ALWAYS_INLINE StringTableImpl& stringTable()
{
return wtfThreadData().atomicStringTable()->table();
}
template<typename T, typename HashTranslator>
-static inline Ref<AtomicStringImpl> addToStringTable(const T& value)
+static inline Ref<AtomicStringImpl> addToStringTable(AtomicStringTableLocker&, StringTableImpl& atomicStringTable, const T& value)
{
- AtomicStringTableLocker locker;
+ auto addResult = atomicStringTable.add<HashTranslator>(value);
- HashSet<StringImpl*>& atomicStringTable = stringTable();
- HashSet<StringImpl*>::AddResult addResult = atomicStringTable.add<HashTranslator>(value);
-
// If the string is newly-translated, then we need to adopt it.
// The boolean in the pair tells us if that is so.
if (addResult.isNewEntry)
@@ -88,6 +87,13 @@
return *static_cast<AtomicStringImpl*>(*addResult.iterator);
}
+template<typename T, typename HashTranslator>
+static inline Ref<AtomicStringImpl> addToStringTable(const T& value)
+{
+ AtomicStringTableLocker locker;
+ return addToStringTable<T, HashTranslator>(locker, stringTable(), value);
+}
+
struct CStringTranslator {
static unsigned hash(const LChar* c)
{
@@ -400,16 +406,30 @@
return addToStringTable<CharBuffer, CharBufferFromLiteralDataTranslator>(buffer);
}
+static inline Ref<AtomicStringImpl> addSubstring(AtomicStringTableLocker& locker, StringTableImpl& atomicStringTable, StringImpl& base)
+{
+ ASSERT(base.length());
+ ASSERT(base.isSymbol() || base.isStatic());
+
+ SubstringLocation buffer = { &base, 0, base.length() };
+ if (base.is8Bit())
+ return addToStringTable<SubstringLocation, SubstringTranslator8>(locker, atomicStringTable, buffer);
+ return addToStringTable<SubstringLocation, SubstringTranslator16>(locker, atomicStringTable, buffer);
+}
+
+static inline Ref<AtomicStringImpl> addSubstring(StringImpl& base)
+{
+ AtomicStringTableLocker locker;
+ return addSubstring(locker, stringTable(), base);
+}
+
Ref<AtomicStringImpl> AtomicStringImpl::addSlowCase(StringImpl& string)
{
if (!string.length())
return *static_cast<AtomicStringImpl*>(StringImpl::empty());
- if (string.isSymbol() || string.isStatic()) {
- if (string.is8Bit())
- return *add(string.characters8(), string.length());
- return *add(string.characters16(), string.length());
- }
+ if (string.isSymbol() || string.isStatic())
+ return addSubstring(string);
ASSERT_WITH_MESSAGE(!string.isAtomic(), "AtomicStringImpl should not hit the slow case if the string is already atomic.");
@@ -430,9 +450,8 @@
return *static_cast<AtomicStringImpl*>(StringImpl::empty());
if (string.isSymbol() || string.isStatic()) {
- if (string.is8Bit())
- return *add(string.characters8(), string.length());
- return *add(string.characters16(), string.length());
+ AtomicStringTableLocker locker;
+ return addSubstring(locker, stringTable.table(), string);
}
ASSERT_WITH_MESSAGE(!string.isAtomic(), "AtomicStringImpl should not hit the slow case if the string is already atomic.");
@@ -452,8 +471,8 @@
{
ASSERT(string->isAtomic());
AtomicStringTableLocker locker;
- HashSet<StringImpl*>& atomicStringTable = stringTable();
- HashSet<StringImpl*>::iterator iterator = atomicStringTable.find(string);
+ auto& atomicStringTable = stringTable();
+ auto iterator = atomicStringTable.find(string);
ASSERT_WITH_MESSAGE(iterator != atomicStringTable.end(), "The string being removed is atomic in the string table of an other thread!");
ASSERT(string == *iterator);
atomicStringTable.remove(iterator);
@@ -466,14 +485,8 @@
if (!string.length())
return static_cast<AtomicStringImpl*>(StringImpl::empty());
- if (string.isSymbol() || string.isStatic()) {
- if (string.is8Bit())
- return lookUpInternal(string.characters8(), string.length());
- return lookUpInternal(string.characters16(), string.length());
- }
-
AtomicStringTableLocker locker;
- HashSet<StringImpl*>& atomicStringTable = stringTable();
+ auto& atomicStringTable = stringTable();
auto iterator = atomicStringTable.find(&string);
if (iterator != atomicStringTable.end())
return static_cast<AtomicStringImpl*>(*iterator);
Modified: trunk/Source/WTF/wtf/text/StringImpl.h (210229 => 210230)
--- trunk/Source/WTF/wtf/text/StringImpl.h 2017-01-03 01:57:40 UTC (rev 210229)
+++ trunk/Source/WTF/wtf/text/StringImpl.h 2017-01-03 02:40:45 UTC (rev 210230)
@@ -537,6 +537,7 @@
}
class StaticStringImpl {
+ WTF_MAKE_NONCOPYABLE(StaticStringImpl);
public:
// Used to construct static strings, which have an special refCount that can never hit zero.
// This means that the static string will never be destroyed, which is important because
@@ -559,6 +560,11 @@
{
}
+ operator StringImpl&()
+ {
+ return *reinterpret_cast<StringImpl*>(this);
+ }
+
// These member variables must match the layout of StringImpl.
unsigned m_refCount;
unsigned m_length;
Modified: trunk/Tools/ChangeLog (210229 => 210230)
--- trunk/Tools/ChangeLog 2017-01-03 01:57:40 UTC (rev 210229)
+++ trunk/Tools/ChangeLog 2017-01-03 02:40:45 UTC (rev 210230)
@@ -1,3 +1,13 @@
+2017-01-02 Yusuke Suzuki <[email protected]>
+
+ Leverage Substring to create new AtomicStringImpl for StaticStringImpl and SymbolImpl
+ https://bugs.webkit.org/show_bug.cgi?id=166636
+
+ Reviewed by Darin Adler.
+
+ * TestWebKitAPI/Tests/WTF/StringImpl.cpp:
+ (TestWebKitAPI::TEST):
+
2017-01-02 Manuel Rego Casasnovas <[email protected]>
[GTK] WebCore/CSSParser unit test is not being built
Modified: trunk/Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp (210229 => 210230)
--- trunk/Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp 2017-01-03 01:57:40 UTC (rev 210229)
+++ trunk/Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp 2017-01-03 02:40:45 UTC (rev 210230)
@@ -554,11 +554,17 @@
ASSERT_TRUE(reference->isSymbol());
ASSERT_FALSE(reference->isAtomic());
+ auto result = AtomicStringImpl::lookUp(reference.ptr());
+ ASSERT_FALSE(result);
+
auto atomic = AtomicStringImpl::add(reference.ptr());
ASSERT_TRUE(atomic->isAtomic());
ASSERT_FALSE(atomic->isSymbol());
ASSERT_TRUE(reference->isSymbol());
ASSERT_FALSE(reference->isAtomic());
+
+ auto result2 = AtomicStringImpl::lookUp(reference.ptr());
+ ASSERT_TRUE(result2);
}
TEST(WTF, StringImplNullSymbolToAtomicString)
@@ -567,13 +573,45 @@
ASSERT_TRUE(reference->isSymbol());
ASSERT_FALSE(reference->isAtomic());
+ // Because the substring of the reference is the empty string which is already interned.
+ auto result = AtomicStringImpl::lookUp(reference.ptr());
+ ASSERT_TRUE(result);
+
auto atomic = AtomicStringImpl::add(reference.ptr());
ASSERT_TRUE(atomic->isAtomic());
ASSERT_FALSE(atomic->isSymbol());
ASSERT_TRUE(reference->isSymbol());
ASSERT_FALSE(reference->isAtomic());
+ ASSERT_EQ(atomic.get(), StringImpl::empty());
+
+ auto result2 = AtomicStringImpl::lookUp(reference.ptr());
+ ASSERT_TRUE(result2);
}
+static StringImpl::StaticStringImpl staticString {"Cocoa"};
+
+TEST(WTF, StringImplStaticToAtomicString)
+{
+ StringImpl& original = staticString;
+ ASSERT_FALSE(original.isSymbol());
+ ASSERT_FALSE(original.isAtomic());
+ ASSERT_TRUE(original.isStatic());
+
+ auto result = AtomicStringImpl::lookUp(&original);
+ ASSERT_FALSE(result);
+
+ auto atomic = AtomicStringImpl::add(&original);
+ ASSERT_TRUE(atomic->isAtomic());
+ ASSERT_FALSE(atomic->isSymbol());
+ ASSERT_FALSE(atomic->isStatic());
+ ASSERT_FALSE(original.isSymbol());
+ ASSERT_FALSE(original.isAtomic());
+ ASSERT_TRUE(original.isStatic());
+
+ auto result2 = AtomicStringImpl::lookUp(&original);
+ ASSERT_TRUE(result2);
+}
+
TEST(WTF, StringImplConstexprHasher)
{
ASSERT_EQ(stringFromUTF8("")->hash(), StringHasher::computeLiteralHashAndMaskTop8Bits(""));
