Leverage Substring to create new AtomicStringImpl for StaticStringImpl and SymbolImpl
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Jan 2017 02:40:45 +0000 (02:40 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Jan 2017 02:40:45 +0000 (02:40 +0000)
https://bugs.webkit.org/show_bug.cgi?id=166636

Reviewed by Darin Adler.

Source/WTF:

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&):

Tools:

* TestWebKitAPI/Tests/WTF/StringImpl.cpp:
(TestWebKitAPI::TEST):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@210230 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/text/AtomicStringImpl.cpp
Source/WTF/wtf/text/StringImpl.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp

index 0271fa7..cd2ef07 100644 (file)
@@ -1,5 +1,31 @@
 2017-01-02  Yusuke Suzuki  <utatane.tea@gmail.com>
 
+        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  <utatane.tea@gmail.com>
+
         Use StaticStringImpl instead of StaticASCIILiteral
         https://bugs.webkit.org/show_bug.cgi?id=166586
 
index af7ccb5..fb50b7f 100644 (file)
@@ -68,18 +68,17 @@ public:
 
 #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;
-
-    HashSet<StringImpl*>& atomicStringTable = stringTable();
-    HashSet<StringImpl*>::AddResult addResult = atomicStringTable.add<HashTranslator>(value);
+    auto 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.
@@ -88,6 +87,13 @@ static inline Ref<AtomicStringImpl> addToStringTable(const T& value)
     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 @@ Ref<AtomicStringImpl> AtomicStringImpl::addLiteral(const char* characters, unsig
     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 @@ Ref<AtomicStringImpl> AtomicStringImpl::addSlowCase(AtomicStringTable& stringTab
         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 @@ void AtomicStringImpl::remove(AtomicStringImpl* string)
 {
     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 @@ RefPtr<AtomicStringImpl> AtomicStringImpl::lookUpSlowCase(StringImpl& string)
     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);
index f76242b..b2c45e8 100644 (file)
@@ -537,6 +537,7 @@ public:
     }
 
     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 @@ public:
         {
         }
 
+        operator StringImpl&()
+        {
+            return *reinterpret_cast<StringImpl*>(this);
+        }
+
         // These member variables must match the layout of StringImpl.
         unsigned m_refCount;
         unsigned m_length;
index e4befee..fc6b20d 100644 (file)
@@ -1,3 +1,13 @@
+2017-01-02  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        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  <rego@igalia.com>
 
         [GTK] WebCore/CSSParser unit test is not being built
index a398f07..fdb701c 100644 (file)
@@ -554,11 +554,17 @@ TEST(WTF, StringImplSymbolToAtomicString)
     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,11 +573,43 @@ TEST(WTF, StringImplNullSymbolToAtomicString)
     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)