Refactor SymbolImpl layout
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 4 Dec 2016 00:28:49 +0000 (00:28 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 4 Dec 2016 00:28:49 +0000 (00:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=165247

Reviewed by Darin Adler.

Source/JavaScriptCore:

Use SymbolImpl::{create, createNullSymbol} instead.

* runtime/PrivateName.h:
(JSC::PrivateName::PrivateName):

Source/WTF:

This patch moves SymbolImpl initialization from StringImpl to SymbolImpl.
In SymbolImpl, we create the appropriate fields. At that time, these fields
should be aligned to the BufferSubstring StringImpl.

And we newly create the `m_flags` in SymbolImpl. Instead of using special
StringImpl::null(), we store s_flagIsNullSymbol flag here. In WTF, we have
the invariant that StringImpl::empty() is the only atomic empty string.
But StringImpl::null() breaks this invariant. Using a special flag is safer
way to represent the null Symbol `Symbol()`.

* WTF.xcodeproj/project.pbxproj:
* wtf/CMakeLists.txt:
* wtf/StdLibExtras.h:
(WTF::roundUpToMultipleOfImpl0):
(WTF::roundUpToMultipleOfImpl):
(WTF::roundUpToMultipleOf):
* wtf/text/StringImpl.cpp:
(WTF::StringImpl::~StringImpl):
(WTF::StringImpl::createSymbol): Deleted.
(WTF::StringImpl::createNullSymbol): Deleted.
* wtf/text/StringImpl.h:
(WTF::StringImpl::isAtomic):
(WTF::StringImpl::StringImpl):
(WTF::StringImpl::requiresCopy):
(WTF::StringImpl::isNullSymbol): Deleted.
(WTF::StringImpl::symbolAwareHash): Deleted.
(WTF::StringImpl::existingSymbolAwareHash): Deleted.
(WTF::StringImpl::null): Deleted.
(WTF::StringImpl::extractFoldedStringInSymbol): Deleted.
(WTF::StringImpl::symbolRegistry): Deleted.
(WTF::StringImpl::hashForSymbol): Deleted.
* wtf/text/StringStatics.cpp:
(WTF::StringImpl::nextHashForSymbol): Deleted.
* wtf/text/SymbolImpl.cpp: Copied from Source/WTF/wtf/text/SymbolRegistry.cpp.
(WTF::SymbolImpl::nextHashForSymbol):
(WTF::SymbolImpl::create):
(WTF::SymbolImpl::createNullSymbol):
* wtf/text/SymbolImpl.h:
(WTF::SymbolImpl::hashForSymbol):
(WTF::SymbolImpl::symbolRegistry):
(WTF::SymbolImpl::isNullSymbol):
(WTF::SymbolImpl::extractFoldedString):
(WTF::SymbolImpl::SymbolImpl):
(WTF::StringImpl::symbolAwareHash):
(WTF::StringImpl::existingSymbolAwareHash):
* wtf/text/SymbolRegistry.cpp:
(WTF::SymbolRegistry::~SymbolRegistry):
(WTF::SymbolRegistry::symbolForKey):
(WTF::SymbolRegistry::keyForSymbol):
* wtf/text/UniquedStringImpl.h:
(WTF::UniquedStringImpl::UniquedStringImpl):

Tools:

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

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

15 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/PrivateName.h
Source/WTF/ChangeLog
Source/WTF/WTF.xcodeproj/project.pbxproj
Source/WTF/wtf/CMakeLists.txt
Source/WTF/wtf/StdLibExtras.h
Source/WTF/wtf/text/StringImpl.cpp
Source/WTF/wtf/text/StringImpl.h
Source/WTF/wtf/text/StringStatics.cpp
Source/WTF/wtf/text/SymbolImpl.cpp [new file with mode: 0644]
Source/WTF/wtf/text/SymbolImpl.h
Source/WTF/wtf/text/SymbolRegistry.cpp
Source/WTF/wtf/text/UniquedStringImpl.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp

index d0ded3e..05e415d 100644 (file)
@@ -1,3 +1,15 @@
+2016-12-03  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        Refactor SymbolImpl layout
+        https://bugs.webkit.org/show_bug.cgi?id=165247
+
+        Reviewed by Darin Adler.
+
+        Use SymbolImpl::{create, createNullSymbol} instead.
+
+        * runtime/PrivateName.h:
+        (JSC::PrivateName::PrivateName):
+
 2016-12-03  JF Bastien  <jfbastien@apple.com>
 
         WebAssembly: update binary format to 0xD version
index 1e1b396..4b86dec 100644 (file)
@@ -32,7 +32,7 @@ namespace JSC {
 class PrivateName {
 public:
     PrivateName()
-        : m_uid(StringImpl::createNullSymbol())
+        : m_uid(SymbolImpl::createNullSymbol())
     {
     }
 
@@ -43,7 +43,7 @@ public:
 
     enum DescriptionTag { Description };
     explicit PrivateName(DescriptionTag, const String& description)
-        : m_uid(StringImpl::createSymbol(*description.impl()))
+        : m_uid(SymbolImpl::create(*description.impl()))
     {
     }
 
index cfca090..4f12ed3 100644 (file)
@@ -1,3 +1,62 @@
+2016-12-03  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        Refactor SymbolImpl layout
+        https://bugs.webkit.org/show_bug.cgi?id=165247
+
+        Reviewed by Darin Adler.
+
+        This patch moves SymbolImpl initialization from StringImpl to SymbolImpl.
+        In SymbolImpl, we create the appropriate fields. At that time, these fields
+        should be aligned to the BufferSubstring StringImpl.
+
+        And we newly create the `m_flags` in SymbolImpl. Instead of using special
+        StringImpl::null(), we store s_flagIsNullSymbol flag here. In WTF, we have
+        the invariant that StringImpl::empty() is the only atomic empty string.
+        But StringImpl::null() breaks this invariant. Using a special flag is safer
+        way to represent the null Symbol `Symbol()`.
+
+        * WTF.xcodeproj/project.pbxproj:
+        * wtf/CMakeLists.txt:
+        * wtf/StdLibExtras.h:
+        (WTF::roundUpToMultipleOfImpl0):
+        (WTF::roundUpToMultipleOfImpl):
+        (WTF::roundUpToMultipleOf):
+        * wtf/text/StringImpl.cpp:
+        (WTF::StringImpl::~StringImpl):
+        (WTF::StringImpl::createSymbol): Deleted.
+        (WTF::StringImpl::createNullSymbol): Deleted.
+        * wtf/text/StringImpl.h:
+        (WTF::StringImpl::isAtomic):
+        (WTF::StringImpl::StringImpl):
+        (WTF::StringImpl::requiresCopy):
+        (WTF::StringImpl::isNullSymbol): Deleted.
+        (WTF::StringImpl::symbolAwareHash): Deleted.
+        (WTF::StringImpl::existingSymbolAwareHash): Deleted.
+        (WTF::StringImpl::null): Deleted.
+        (WTF::StringImpl::extractFoldedStringInSymbol): Deleted.
+        (WTF::StringImpl::symbolRegistry): Deleted.
+        (WTF::StringImpl::hashForSymbol): Deleted.
+        * wtf/text/StringStatics.cpp:
+        (WTF::StringImpl::nextHashForSymbol): Deleted.
+        * wtf/text/SymbolImpl.cpp: Copied from Source/WTF/wtf/text/SymbolRegistry.cpp.
+        (WTF::SymbolImpl::nextHashForSymbol):
+        (WTF::SymbolImpl::create):
+        (WTF::SymbolImpl::createNullSymbol):
+        * wtf/text/SymbolImpl.h:
+        (WTF::SymbolImpl::hashForSymbol):
+        (WTF::SymbolImpl::symbolRegistry):
+        (WTF::SymbolImpl::isNullSymbol):
+        (WTF::SymbolImpl::extractFoldedString):
+        (WTF::SymbolImpl::SymbolImpl):
+        (WTF::StringImpl::symbolAwareHash):
+        (WTF::StringImpl::existingSymbolAwareHash):
+        * wtf/text/SymbolRegistry.cpp:
+        (WTF::SymbolRegistry::~SymbolRegistry):
+        (WTF::SymbolRegistry::symbolForKey):
+        (WTF::SymbolRegistry::keyForSymbol):
+        * wtf/text/UniquedStringImpl.h:
+        (WTF::UniquedStringImpl::UniquedStringImpl):
+
 2016-12-01  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         Introduce StringImpl::StaticStringImpl with constexpr constructor
index d36a2dd..f5c04d3 100644 (file)
                FE8925B01D00DAEC0046907E /* Indenter.h in Headers */ = {isa = PBXBuildFile; fileRef = FE8925AF1D00DAEC0046907E /* Indenter.h */; };
                FEDACD3D1630F83F00C69634 /* StackStats.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FEDACD3B1630F83F00C69634 /* StackStats.cpp */; };
                FEDACD3E1630F83F00C69634 /* StackStats.h in Headers */ = {isa = PBXBuildFile; fileRef = FEDACD3C1630F83F00C69634 /* StackStats.h */; };
+               52183012C99E476A84EEBEA8 /* SymbolImpl.cpp in Sources */ = {isa = PBXBuildFile; fileRef = F72BBDB107FA424886178B9E /* SymbolImpl.cpp */; };
 /* End PBXBuildFile section */
 
 /* Begin PBXContainerItemProxy section */
                FE8925AF1D00DAEC0046907E /* Indenter.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = Indenter.h; sourceTree = "<group>"; };
                FEDACD3B1630F83F00C69634 /* StackStats.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = StackStats.cpp; sourceTree = "<group>"; };
                FEDACD3C1630F83F00C69634 /* StackStats.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = StackStats.h; sourceTree = "<group>"; };
+               F72BBDB107FA424886178B9E /* SymbolImpl.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = SymbolImpl.cpp; path = SymbolImpl.cpp; sourceTree = "<group>"; };
 /* End PBXFileReference section */
 
 /* Begin PBXFrameworksBuildPhase section */
                                70ECA60C1B02426800449739 /* UniquedStringImpl.h */,
                                A8A4732D151A825B004123FF /* WTFString.cpp */,
                                A8A4732E151A825B004123FF /* WTFString.h */,
+                               F72BBDB107FA424886178B9E /* SymbolImpl.cpp */,
                        );
                        path = text;
                        sourceTree = "<group>";
                                E4A0AD3D1A96253C00536DF6 /* WorkQueueCocoa.cpp in Sources */,
                                A8A47445151A825B004123FF /* WTFString.cpp in Sources */,
                                A8A47486151A825B004123FF /* WTFThreadData.cpp in Sources */,
+                               52183012C99E476A84EEBEA8 /* SymbolImpl.cpp in Sources */,
                        );
                        runOnlyForDeploymentPostprocessing = 0;
                };
index a083f3a..c0513f1 100644 (file)
@@ -246,6 +246,7 @@ set(WTF_SOURCES
     text/StringImpl.cpp
     text/StringStatics.cpp
     text/StringView.cpp
+    text/SymbolImpl.cpp
     text/SymbolRegistry.cpp
     text/TextBreakIterator.cpp
     text/WTFString.cpp
index 5f388eb..64524fd 100644 (file)
@@ -186,18 +186,27 @@ template<typename T> char (&ArrayLengthHelperFunction(T (&)[0]))[0];
 #endif
 #define WTF_ARRAY_LENGTH(array) sizeof(::WTF::ArrayLengthHelperFunction(array))
 
+ALWAYS_INLINE constexpr size_t roundUpToMultipleOfImpl0(size_t remainderMask, size_t x)
+{
+    return (x + remainderMask) & ~remainderMask;
+}
+
+ALWAYS_INLINE constexpr size_t roundUpToMultipleOfImpl(size_t divisor, size_t x)
+{
+    return roundUpToMultipleOfImpl0(divisor - 1, x);
+}
+
 // Efficient implementation that takes advantage of powers of two.
 inline size_t roundUpToMultipleOf(size_t divisor, size_t x)
 {
     ASSERT(divisor && !(divisor & (divisor - 1)));
-    size_t remainderMask = divisor - 1;
-    return (x + remainderMask) & ~remainderMask;
+    return roundUpToMultipleOfImpl(divisor, x);
 }
 
-template<size_t divisor> inline size_t roundUpToMultipleOf(size_t x)
+template<size_t divisor> inline constexpr size_t roundUpToMultipleOf(size_t x)
 {
     static_assert(divisor && !(divisor & (divisor - 1)), "divisor must be a power of two!");
-    return roundUpToMultipleOf(divisor, x);
+    return roundUpToMultipleOfImpl(divisor, x);
 }
 
 enum BinarySearchMode {
index 7d0a701..db39ed6 100644 (file)
@@ -101,7 +101,6 @@ void StringStats::printStats()
 }
 #endif
 
-StringImpl::StaticStringImpl StringImpl::s_atomicNullString("", StringImpl::StringAtomic);
 StringImpl::StaticStringImpl StringImpl::s_atomicEmptyString("", StringImpl::StringAtomic);
 
 StringImpl::~StringImpl()
@@ -115,8 +114,12 @@ StringImpl::~StringImpl()
     if (isAtomic() && length() && !isSymbol())
         AtomicStringImpl::remove(static_cast<AtomicStringImpl*>(this));
 
-    if (isSymbol() && symbolRegistry())
-        symbolRegistry()->remove(static_cast<SymbolImpl&>(*this));
+    if (isSymbol()) {
+        auto& symbol = static_cast<SymbolImpl&>(*this);
+        auto* symbolRegistry = symbol.symbolRegistry();
+        if (symbolRegistry)
+            symbolRegistry->remove(symbol);
+    }
 
     BufferOwnership ownership = bufferOwnership();
 
@@ -292,26 +295,6 @@ Ref<StringImpl> StringImpl::create(const LChar* string)
     return create(string, length);
 }
 
-Ref<SymbolImpl> StringImpl::createSymbol(StringImpl& rep)
-{
-    auto* ownerRep = (rep.bufferOwnership() == BufferSubstring) ? rep.substringBuffer() : &rep;
-
-    // We allocate a buffer that contains
-    // 1. the StringImpl struct
-    // 2. the pointer to the owner string
-    // 3. the pointer to the symbol registry
-    // 4. the placeholder for symbol aware hash value (allocated size is pointer size, but only 4 bytes are used)
-    auto* stringImpl = static_cast<StringImpl*>(fastMalloc(allocationSize<StringImpl*>(3)));
-    if (rep.is8Bit())
-        return adoptRef(static_cast<SymbolImpl&>(*new (NotNull, stringImpl) StringImpl(CreateSymbol, rep.m_data8, rep.length(), *ownerRep)));
-    return adoptRef(static_cast<SymbolImpl&>(*new (NotNull, stringImpl) StringImpl(CreateSymbol, rep.m_data16, rep.length(), *ownerRep)));
-}
-
-Ref<SymbolImpl> StringImpl::createNullSymbol()
-{
-    return createSymbol(*null());
-}
-
 bool StringImpl::containsOnlyWhitespace()
 {
     // FIXME: The definition of whitespace here includes a number of characters
index 8e0f3e7..23b942b 100644 (file)
@@ -139,6 +139,7 @@ class StringImpl {
     friend struct WTF::UCharBufferTranslator;
     friend class JSC::LLInt::Data;
     friend class JSC::LLIntOffsetsExtractor;
+    friend class SymbolImpl;
     
 private:
     enum BufferOwnership {
@@ -281,43 +282,6 @@ private:
         STRING_STATS_ADD_16BIT_STRING2(m_length, true);
     }
 
-    enum CreateSymbolTag { CreateSymbol };
-    // Used to create new symbol strings that holds existing 8-bit [[Description]] string as a substring buffer (BufferSubstring).
-    StringImpl(CreateSymbolTag, const LChar* characters, unsigned length, Ref<StringImpl>&& base)
-        : m_refCount(s_refCountIncrement)
-        , m_length(length)
-        , m_data8(characters)
-        , m_hashAndFlags(s_hashFlag8BitBuffer | StringSymbol | BufferSubstring)
-    {
-        ASSERT(is8Bit());
-        ASSERT(m_data8);
-        ASSERT(base->bufferOwnership() != BufferSubstring);
-
-        substringBuffer() = &base.leakRef();
-        symbolRegistry() = nullptr;
-        hashForSymbol() = nextHashForSymbol();
-
-        STRING_STATS_ADD_8BIT_STRING2(m_length, true);
-    }
-
-    // Used to create new symbol strings that holds existing 16-bit [[Description]] string as a substring buffer (BufferSubstring).
-    StringImpl(CreateSymbolTag, const UChar* characters, unsigned length, Ref<StringImpl>&& base)
-        : m_refCount(s_refCountIncrement)
-        , m_length(length)
-        , m_data16(characters)
-        , m_hashAndFlags(StringSymbol | BufferSubstring)
-    {
-        ASSERT(!is8Bit());
-        ASSERT(m_data16);
-        ASSERT(base->bufferOwnership() != BufferSubstring);
-
-        substringBuffer() = &base.leakRef();
-        symbolRegistry() = nullptr;
-        hashForSymbol() = nextHashForSymbol();
-
-        STRING_STATS_ADD_16BIT_STRING2(m_length, true);
-    }
-
 public:
     WTF_EXPORT_STRING_API static void destroy(StringImpl*);
 
@@ -390,9 +354,6 @@ public:
         return constructInternal<T>(resultImpl, length);
     }
 
-    WTF_EXPORT_STRING_API static Ref<SymbolImpl> createNullSymbol();
-    WTF_EXPORT_STRING_API static Ref<SymbolImpl> createSymbol(StringImpl& rep);
-
     // Reallocate the StringImpl. The originalString must be only owned by the Ref,
     // and the buffer ownership must be BufferInternal. Just like the input pointer of realloc(),
     // the originalString can't be used after this function.
@@ -466,7 +427,6 @@ public:
     StringKind stringKind() const { return static_cast<StringKind>(m_hashAndFlags & s_hashMaskStringKind); }
     bool isSymbol() const { return m_hashAndFlags & s_hashFlagStringKindIsSymbol; }
     bool isAtomic() const { return m_hashAndFlags & s_hashFlagStringKindIsAtomic; }
-    bool isNullSymbol() const { return isSymbol() && (substringBuffer() == null()); }
 
     void setIsAtomic(bool isAtomic)
     {
@@ -536,19 +496,8 @@ public:
 
     WTF_EXPORT_PRIVATE unsigned concurrentHash() const;
 
-    unsigned symbolAwareHash() const
-    {
-        if (isSymbol())
-            return hashForSymbol();
-        return hash();
-    }
-
-    unsigned existingSymbolAwareHash() const
-    {
-        if (isSymbol())
-            return hashForSymbol();
-        return existingHash();
-    }
+    unsigned symbolAwareHash() const;
+    unsigned existingSymbolAwareHash() const;
 
     bool isStatic() const { return m_refCount & s_refCountFlagIsStaticString; }
 
@@ -620,9 +569,7 @@ public:
         unsigned m_hashAndFlags;
     };
 
-    WTF_EXPORTDATA static StaticStringImpl s_atomicNullString;
     WTF_EXPORTDATA static StaticStringImpl s_atomicEmptyString;
-    ALWAYS_INLINE static StringImpl* null() { return reinterpret_cast<StringImpl*>(&s_atomicNullString); }
     ALWAYS_INLINE static StringImpl* empty() { return reinterpret_cast<StringImpl*>(&s_atomicEmptyString); }
 
     // FIXME: Does this really belong in StringImpl?
@@ -778,50 +725,45 @@ public:
     ALWAYS_INLINE static StringStats& stringStats() { return m_stringStats; }
 #endif
 
-    Ref<StringImpl> extractFoldedStringInSymbol()
-    {
-        ASSERT(isSymbol());
-        ASSERT(bufferOwnership() == BufferSubstring);
-        ASSERT(substringBuffer());
-        ASSERT(!substringBuffer()->isSymbol());
-        return createSubstringSharingImpl(*this, 0, length());
-    }
-
-    SymbolRegistry* const& symbolRegistry() const
-    {
-        ASSERT(isSymbol());
-        return *(tailPointer<SymbolRegistry*>() + 1);
-    }
+protected:
+    ~StringImpl();
 
-    SymbolRegistry*& symbolRegistry()
-    {
-        ASSERT(isSymbol());
-        return *(tailPointer<SymbolRegistry*>() + 1);
-    }
+    enum CreateSymbolTag { CreateSymbol };
 
-    const unsigned& hashForSymbol() const
+    // Used to create new symbol strings that holds existing 8-bit [[Description]] string as a substring buffer (BufferSubstring).
+    StringImpl(CreateSymbolTag, const LChar* characters, unsigned length)
+        : m_refCount(s_refCountIncrement)
+        , m_length(length)
+        , m_data8(characters)
+        , m_hashAndFlags(s_hashFlag8BitBuffer | StringSymbol | BufferSubstring)
     {
-        return const_cast<StringImpl*>(this)->hashForSymbol();
+        ASSERT(is8Bit());
+        ASSERT(m_data8);
+        STRING_STATS_ADD_8BIT_STRING2(m_length, true);
     }
 
-    unsigned& hashForSymbol()
+    // Used to create new symbol strings that holds existing 16-bit [[Description]] string as a substring buffer (BufferSubstring).
+    StringImpl(CreateSymbolTag, const UChar* characters, unsigned length)
+        : m_refCount(s_refCountIncrement)
+        , m_length(length)
+        , m_data16(characters)
+        , m_hashAndFlags(StringSymbol | BufferSubstring)
     {
-        ASSERT(isSymbol());
-        return *reinterpret_cast<unsigned*>((tailPointer<SymbolRegistry*>() + 2));
+        ASSERT(!is8Bit());
+        ASSERT(m_data16);
+        STRING_STATS_ADD_16BIT_STRING2(m_length, true);
     }
 
-protected:
-    ~StringImpl();
-
-private:
-    bool requiresCopy() const
+    // Null symbol.
+    StringImpl(CreateSymbolTag)
+        : m_refCount(s_refCountIncrement)
+        , m_length(0)
+        , m_data8(empty()->characters8())
+        , m_hashAndFlags(s_hashFlag8BitBuffer | StringSymbol | BufferSubstring)
     {
-        if (bufferOwnership() != BufferInternal)
-            return true;
-
-        if (is8Bit())
-            return m_data8 == tailPointer<LChar>();
-        return m_data16 == tailPointer<UChar>();
+        ASSERT(is8Bit());
+        ASSERT(m_data8);
+        STRING_STATS_ADD_8BIT_STRING2(m_length, true);
     }
 
     template<typename T>
@@ -841,6 +783,17 @@ private:
 #endif
     }
 
+private:
+    bool requiresCopy() const
+    {
+        if (bufferOwnership() != BufferInternal)
+            return true;
+
+        if (is8Bit())
+            return m_data8 == tailPointer<LChar>();
+        return m_data16 == tailPointer<UChar>();
+    }
+
     template<typename T>
     const T* tailPointer() const
     {
@@ -882,7 +835,6 @@ private:
     template <typename CharType> static Ref<StringImpl> reallocateInternal(Ref<StringImpl>&&, unsigned, CharType*&);
     template <typename CharType> static Ref<StringImpl> createInternal(const CharType*, unsigned);
     WTF_EXPORT_PRIVATE NEVER_INLINE unsigned hashSlowCase() const;
-    WTF_EXPORT_PRIVATE static unsigned nextHashForSymbol();
 
     // The bottom bit in the ref count indicates a static (immortal) string.
     static const unsigned s_refCountFlagIsStaticString = 0x1;
index 0d254a5..0c2119c 100644 (file)
 
 namespace WTF {
 
-// In addition to the normal hash value, store specialized hash value for
-// symbolized StringImpl*. And don't use the normal hash value for symbolized
-// StringImpl* when they are treated as Identifiers. Unique nature of these
-// symbolized StringImpl* keys means that we don't need them to match any other
-// string (in fact, that's exactly the oposite of what we want!), and the
-// normal hash would lead to lots of conflicts.
-unsigned StringImpl::nextHashForSymbol()
-{
-    static unsigned s_nextHashForSymbol = 0;
-    s_nextHashForSymbol += 1 << s_flagCount;
-    s_nextHashForSymbol |= 1 << 31;
-    return s_nextHashForSymbol;
-}
-
 WTF_EXPORTDATA DEFINE_GLOBAL(AtomicString, nullAtom)
 WTF_EXPORTDATA DEFINE_GLOBAL(AtomicString, emptyAtom)
 WTF_EXPORTDATA DEFINE_GLOBAL(AtomicString, starAtom)
diff --git a/Source/WTF/wtf/text/SymbolImpl.cpp b/Source/WTF/wtf/text/SymbolImpl.cpp
new file mode 100644 (file)
index 0000000..18ebea9
--- /dev/null
@@ -0,0 +1,59 @@
+/*
+ * Copyright (C) 2016 Yusuke Suzuki <utatane.tea@gmail.com>.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+#include "SymbolImpl.h"
+
+namespace WTF {
+
+// In addition to the normal hash value, store specialized hash value for
+// symbolized StringImpl*. And don't use the normal hash value for symbolized
+// StringImpl* when they are treated as Identifiers. Unique nature of these
+// symbolized StringImpl* keys means that we don't need them to match any other
+// string (in fact, that's exactly the oposite of what we want!), and the
+// normal hash would lead to lots of conflicts.
+unsigned SymbolImpl::nextHashForSymbol()
+{
+    static unsigned s_nextHashForSymbol = 0;
+    s_nextHashForSymbol += 1 << s_flagCount;
+    s_nextHashForSymbol |= 1 << 31;
+    return s_nextHashForSymbol;
+}
+
+Ref<SymbolImpl> SymbolImpl::create(StringImpl& rep)
+{
+    auto* ownerRep = (rep.bufferOwnership() == BufferSubstring) ? rep.substringBuffer() : &rep;
+    ASSERT(ownerRep->bufferOwnership() != BufferSubstring);
+    if (rep.is8Bit())
+        return adoptRef(*new SymbolImpl(rep.m_data8, rep.length(), *ownerRep));
+    return adoptRef(*new SymbolImpl(rep.m_data16, rep.length(), *ownerRep));
+}
+
+Ref<SymbolImpl> SymbolImpl::createNullSymbol()
+{
+    return adoptRef(*new SymbolImpl);
+}
+
+} // namespace WTF
index 4043bf4..8bacf9b 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015 Yusuke Suzuki <utatane.tea@gmail.com>.
+ * Copyright (C) 2015-2016 Yusuke Suzuki <utatane.tea@gmail.com>.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -23,8 +23,7 @@
  * THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#ifndef SymbolImpl_h
-#define SymbolImpl_h
+#pragma once
 
 #include <wtf/text/UniquedStringImpl.h>
 
@@ -34,9 +33,77 @@ namespace WTF {
 // It is uniqued string impl, but is not registered in Atomic String tables, so it's not atomic.
 class SymbolImpl : public UniquedStringImpl {
 private:
-    SymbolImpl() = delete;
+    static constexpr const unsigned s_flagIsNullSymbol = 1u;
+
+public:
+    unsigned hashForSymbol() const { return m_hashForSymbol; }
+    SymbolRegistry* const& symbolRegistry() const { return m_symbolRegistry; }
+    SymbolRegistry*& symbolRegistry() { return m_symbolRegistry; }
+    bool isNullSymbol() const { return m_flags & s_flagIsNullSymbol; }
+
+    WTF_EXPORT_STRING_API static Ref<SymbolImpl> createNullSymbol();
+    WTF_EXPORT_STRING_API static Ref<SymbolImpl> create(StringImpl& rep);
+
+    Ref<StringImpl> extractFoldedString()
+    {
+        ASSERT(substringBuffer());
+        ASSERT(substringBuffer() == m_owner);
+        ASSERT(!substringBuffer()->isSymbol());
+        return createSubstringSharingImpl(*this, 0, length());
+    }
+
+private:
+    WTF_EXPORT_PRIVATE static unsigned nextHashForSymbol();
+
+    friend class StringImpl;
+
+    SymbolImpl(const LChar* characters, unsigned length, Ref<StringImpl>&& base)
+        : UniquedStringImpl(CreateSymbol, characters, length)
+        , m_owner(&base.leakRef())
+        , m_hashForSymbol(nextHashForSymbol())
+    {
+        ASSERT(StringImpl::tailOffset<StringImpl*>() == OBJECT_OFFSETOF(SymbolImpl, m_owner));
+    }
+
+    SymbolImpl(const UChar* characters, unsigned length, Ref<StringImpl>&& base)
+        : UniquedStringImpl(CreateSymbol, characters, length)
+        , m_owner(&base.leakRef())
+        , m_hashForSymbol(nextHashForSymbol())
+    {
+        ASSERT(StringImpl::tailOffset<StringImpl*>() == OBJECT_OFFSETOF(SymbolImpl, m_owner));
+    }
+
+    SymbolImpl()
+        : UniquedStringImpl(CreateSymbol)
+        , m_owner(StringImpl::empty())
+        , m_hashForSymbol(nextHashForSymbol())
+        , m_flags(s_flagIsNullSymbol)
+    {
+        ASSERT(StringImpl::tailOffset<StringImpl*>() == OBJECT_OFFSETOF(SymbolImpl, m_owner));
+    }
+
+    // The pointer to the owner string should be immediately following after the StringImpl layout,
+    // since we would like to align the layout of SymbolImpl to the one of BufferSubstring StringImpl.
+    StringImpl* m_owner;
+    SymbolRegistry* m_symbolRegistry { nullptr };
+    unsigned m_hashForSymbol;
+    unsigned m_flags { 0 };
 };
 
+inline unsigned StringImpl::symbolAwareHash() const
+{
+    if (isSymbol())
+        return static_cast<const SymbolImpl*>(this)->hashForSymbol();
+    return hash();
+}
+
+inline unsigned StringImpl::existingSymbolAwareHash() const
+{
+    if (isSymbol())
+        return static_cast<const SymbolImpl*>(this)->hashForSymbol();
+    return existingHash();
+}
+
 #if !ASSERT_DISABLED
 // SymbolImpls created from StaticASCIILiteral will ASSERT
 // in the generic ValueCheck<T>::checkConsistency
@@ -57,5 +124,3 @@ ValueCheck<const SymbolImpl*> {
 } // namespace WTF
 
 using WTF::SymbolImpl;
-
-#endif // SymbolImpl_h
index 64ea1ca..264bc5c 100644 (file)
@@ -31,7 +31,7 @@ namespace WTF {
 SymbolRegistry::~SymbolRegistry()
 {
     for (auto& key : m_table)
-        key.impl()->symbolRegistry() = nullptr;
+        static_cast<SymbolImpl&>(*key.impl()).symbolRegistry() = nullptr;
 }
 
 Ref<SymbolImpl> SymbolRegistry::symbolForKey(const String& rep)
@@ -40,7 +40,7 @@ Ref<SymbolImpl> SymbolRegistry::symbolForKey(const String& rep)
     if (!addResult.isNewEntry)
         return *static_cast<SymbolImpl*>(addResult.iterator->impl());
 
-    auto symbol = StringImpl::createSymbol(*rep.impl());
+    auto symbol = SymbolImpl::create(*rep.impl());
     symbol->symbolRegistry() = this;
     *addResult.iterator = SymbolRegistryKey(&symbol.get());
     return symbol;
@@ -49,7 +49,7 @@ Ref<SymbolImpl> SymbolRegistry::symbolForKey(const String& rep)
 String SymbolRegistry::keyForSymbol(SymbolImpl& uid)
 {
     ASSERT(uid.symbolRegistry() == this);
-    return uid.extractFoldedStringInSymbol();
+    return uid.extractFoldedString();
 }
 
 void SymbolRegistry::remove(SymbolImpl& uid)
index 554e533..b3de0e5 100644 (file)
@@ -35,6 +35,10 @@ namespace WTF {
 class UniquedStringImpl : public StringImpl {
 private:
     UniquedStringImpl() = delete;
+protected:
+    UniquedStringImpl(CreateSymbolTag, const LChar* characters, unsigned length) : StringImpl(CreateSymbol, characters, length) { }
+    UniquedStringImpl(CreateSymbolTag, const UChar* characters, unsigned length) : StringImpl(CreateSymbol, characters, length) { }
+    UniquedStringImpl(CreateSymbolTag) : StringImpl(CreateSymbol) { }
 };
 
 #if !ASSERT_DISABLED
index 384f31d..6216b72 100644 (file)
@@ -1,3 +1,13 @@
+2016-12-03  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        Refactor SymbolImpl layout
+        https://bugs.webkit.org/show_bug.cgi?id=165247
+
+        Reviewed by Darin Adler.
+
+        * TestWebKitAPI/Tests/WTF/StringImpl.cpp:
+        (TestWebKitAPI::TEST):
+
 2016-12-03  Dan Bernstein  <mitz@apple.com>
 
         Fixed the build after r209307.
index d3710fa..a398f07 100644 (file)
@@ -516,7 +516,7 @@ TEST(WTF, StringImplEndsWithIgnoringASCIICaseWithEmpty)
 
 TEST(WTF, StringImplCreateNullSymbol)
 {
-    auto reference = StringImpl::createNullSymbol();
+    auto reference = SymbolImpl::createNullSymbol();
     ASSERT_TRUE(reference->isSymbol());
     ASSERT_TRUE(reference->isNullSymbol());
     ASSERT_FALSE(reference->isAtomic());
@@ -527,7 +527,7 @@ TEST(WTF, StringImplCreateNullSymbol)
 TEST(WTF, StringImplCreateSymbol)
 {
     auto original = stringFromUTF8("original");
-    auto reference = StringImpl::createSymbol(original);
+    auto reference = SymbolImpl::create(original);
     ASSERT_TRUE(reference->isSymbol());
     ASSERT_FALSE(reference->isNullSymbol());
     ASSERT_FALSE(reference->isAtomic());
@@ -537,12 +537,11 @@ TEST(WTF, StringImplCreateSymbol)
     ASSERT_TRUE(equal(reference.ptr(), "original"));
 
     auto empty = stringFromUTF8("");
-    auto emptyReference = StringImpl::createSymbol(empty);
+    auto emptyReference = SymbolImpl::create(empty);
     ASSERT_TRUE(emptyReference->isSymbol());
     ASSERT_FALSE(emptyReference->isNullSymbol());
     ASSERT_FALSE(emptyReference->isAtomic());
     ASSERT_FALSE(empty->isSymbol());
-    ASSERT_FALSE(empty->isNullSymbol());
     ASSERT_TRUE(empty->isAtomic());
     ASSERT_EQ(empty->length(), emptyReference->length());
     ASSERT_TRUE(equal(emptyReference.ptr(), ""));
@@ -551,7 +550,7 @@ TEST(WTF, StringImplCreateSymbol)
 TEST(WTF, StringImplSymbolToAtomicString)
 {
     auto original = stringFromUTF8("original");
-    auto reference = StringImpl::createSymbol(original);
+    auto reference = SymbolImpl::create(original);
     ASSERT_TRUE(reference->isSymbol());
     ASSERT_FALSE(reference->isAtomic());
 
@@ -564,7 +563,7 @@ TEST(WTF, StringImplSymbolToAtomicString)
 
 TEST(WTF, StringImplNullSymbolToAtomicString)
 {
-    auto reference = StringImpl::createNullSymbol();
+    auto reference = SymbolImpl::createNullSymbol();
     ASSERT_TRUE(reference->isSymbol());
     ASSERT_FALSE(reference->isAtomic());
 
@@ -587,8 +586,6 @@ TEST(WTF, StringImplConstexprHasher)
 TEST(WTF, StringImplEmpty)
 {
     ASSERT_FALSE(StringImpl::empty()->length());
-    ASSERT_FALSE(StringImpl::null()->length());
-    ASSERT_NE(StringImpl::null(), StringImpl::empty());
 }
 
 } // namespace TestWebKitAPI