[JSC] SmallStringsStorage is unnecessary
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Feb 2019 19:04:40 +0000 (19:04 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Feb 2019 19:04:40 +0000 (19:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194939

Reviewed by Mark Lam.

SmallStrings hold common small JSStrings. Their underlying StringImpl is also held by SmallStringsStorage.
But it is duplicate since we can get StringImpl from small JSStrings. This patch removes SmallStringsStorage,
and get StringImpls from JSStrings if necessary.

We also add m_canAccessHeap flag to SmallStrings. At the time of VM destruction, JSStrings are destroyed when
VM's Heap is finalized. We must not touch JSStrings before VM's heap (and JSStrings in SmallStrings) is initialized,
and after VM's Heap is destroyed. We add this m_canAccessHeap flag to allow users to get StringImpl during the
this sensitive period. If m_canAccessHeap is false, we get StringImpl from AtomicStringImpl::add.

* runtime/SmallStrings.cpp:
(JSC::SmallStrings::initializeCommonStrings):
(JSC::SmallStrings::singleCharacterStringRep):
(JSC::SmallStringsStorage::rep): Deleted.
(JSC::SmallStringsStorage::SmallStringsStorage): Deleted.
(JSC::SmallStrings::createSingleCharacterString): Deleted.
* runtime/SmallStrings.h:
(JSC::SmallStrings::setCanAccessHeap):
* runtime/VM.cpp:
(JSC::VM::VM):
(JSC::VM::~VM):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/SmallStrings.cpp
Source/JavaScriptCore/runtime/SmallStrings.h
Source/JavaScriptCore/runtime/VM.cpp

index fc61627..2ef673b 100644 (file)
@@ -1,3 +1,31 @@
+2019-02-22  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] SmallStringsStorage is unnecessary
+        https://bugs.webkit.org/show_bug.cgi?id=194939
+
+        Reviewed by Mark Lam.
+
+        SmallStrings hold common small JSStrings. Their underlying StringImpl is also held by SmallStringsStorage.
+        But it is duplicate since we can get StringImpl from small JSStrings. This patch removes SmallStringsStorage,
+        and get StringImpls from JSStrings if necessary.
+
+        We also add m_canAccessHeap flag to SmallStrings. At the time of VM destruction, JSStrings are destroyed when
+        VM's Heap is finalized. We must not touch JSStrings before VM's heap (and JSStrings in SmallStrings) is initialized,
+        and after VM's Heap is destroyed. We add this m_canAccessHeap flag to allow users to get StringImpl during the
+        this sensitive period. If m_canAccessHeap is false, we get StringImpl from AtomicStringImpl::add.
+
+        * runtime/SmallStrings.cpp:
+        (JSC::SmallStrings::initializeCommonStrings):
+        (JSC::SmallStrings::singleCharacterStringRep):
+        (JSC::SmallStringsStorage::rep): Deleted.
+        (JSC::SmallStringsStorage::SmallStringsStorage): Deleted.
+        (JSC::SmallStrings::createSingleCharacterString): Deleted.
+        * runtime/SmallStrings.h:
+        (JSC::SmallStrings::setCanAccessHeap):
+        * runtime/VM.cpp:
+        (JSC::VM::VM):
+        (JSC::VM::~VM):
+
 2019-02-22  Tadeu Zagallo  <tzagallo@apple.com>
 
         Cache CompactVariableMap::Handle instead of VariableEnvironment for UnlinkedFunctionExecutable
index 8f26b3e..54fdab9 100644 (file)
 
 namespace JSC {
 
-class SmallStringsStorage {
-    WTF_MAKE_NONCOPYABLE(SmallStringsStorage); WTF_MAKE_FAST_ALLOCATED;
-public:
-    SmallStringsStorage();
-
-    StringImpl& rep(unsigned char character)
-    {
-        return *m_reps[character].get();
-    }
-
-private:
-    static const unsigned singleCharacterStringCount = maxSingleCharacterString + 1;
-
-    RefPtr<StringImpl> m_reps[singleCharacterStringCount];
-};
-
-SmallStringsStorage::SmallStringsStorage()
-{
-    for (unsigned i = 0; i < singleCharacterStringCount; ++i) {
-        const LChar string[] = { static_cast<LChar>(i) };
-        m_reps[i] = AtomicStringImpl::add(StringImpl::create(string, 1).ptr());
-    }
-}
-
 SmallStrings::SmallStrings()
 {
     COMPILE_ASSERT(singleCharacterStringCount == sizeof(m_singleCharacterStrings) / sizeof(m_singleCharacterStrings[0]), IsNumCharactersConstInSyncWithClassUsage);
@@ -69,14 +45,22 @@ SmallStrings::SmallStrings()
 void SmallStrings::initializeCommonStrings(VM& vm)
 {
     createEmptyString(&vm);
-    for (unsigned i = 0; i <= maxSingleCharacterString; ++i)
-        createSingleCharacterString(&vm, i);
+
+    for (unsigned i = 0; i < singleCharacterStringCount; ++i) {
+        ASSERT(!m_singleCharacterStrings[i]);
+        const LChar string[] = { static_cast<LChar>(i) };
+        m_singleCharacterStrings[i] = JSString::createHasOtherOwner(vm, AtomicStringImpl::add(string, 1).releaseNonNull());
+        ASSERT(m_needsToBeVisited);
+    }
+
 #define JSC_COMMON_STRINGS_ATTRIBUTE_INITIALIZE(name) initialize(&vm, m_##name, #name);
     JSC_COMMON_STRINGS_EACH_NAME(JSC_COMMON_STRINGS_ATTRIBUTE_INITIALIZE)
 #undef JSC_COMMON_STRINGS_ATTRIBUTE_INITIALIZE
     initialize(&vm, m_objectStringStart, "[object ");
     initialize(&vm, m_nullObjectString, "[object Null]");
     initialize(&vm, m_undefinedObjectString, "[object Undefined]");
+
+    setInitialized(true);
 }
 
 void SmallStrings::visitStrongReferences(SlotVisitor& visitor)
@@ -104,20 +88,12 @@ void SmallStrings::createEmptyString(VM* vm)
     ASSERT(m_needsToBeVisited);
 }
 
-void SmallStrings::createSingleCharacterString(VM* vm, unsigned char character)
-{
-    if (!m_storage)
-        m_storage = std::make_unique<SmallStringsStorage>();
-    ASSERT(!m_singleCharacterStrings[character]);
-    m_singleCharacterStrings[character] = JSString::createHasOtherOwner(*vm, m_storage->rep(character));
-    ASSERT(m_needsToBeVisited);
-}
-
-StringImpl& SmallStrings::singleCharacterStringRep(unsigned char character)
+Ref<StringImpl> SmallStrings::singleCharacterStringRep(unsigned char character)
 {
-    if (!m_storage)
-        m_storage = std::make_unique<SmallStringsStorage>();
-    return m_storage->rep(character);
+    if (LIKELY(m_isInitialized))
+        return *const_cast<StringImpl*>(m_singleCharacterStrings[character]->tryGetValueImpl());
+    const LChar string[] = { static_cast<LChar>(character) };
+    return AtomicStringImpl::add(string, 1).releaseNonNull();
 }
 
 void SmallStrings::initialize(VM* vm, JSString*& string, const char* value)
index 769d494..deead73 100644 (file)
@@ -51,7 +51,6 @@ namespace JSC {
 
 class VM;
 class JSString;
-class SmallStringsStorage;
 class SlotVisitor;
 
 static const unsigned maxSingleCharacterString = 0xFF;
@@ -72,7 +71,9 @@ public:
         return m_singleCharacterStrings[character];
     }
 
-    JS_EXPORT_PRIVATE WTF::StringImpl& singleCharacterStringRep(unsigned char character);
+    JS_EXPORT_PRIVATE Ref<StringImpl> singleCharacterStringRep(unsigned char character);
+
+    void setIsInitialized(bool isInitialized) { m_isInitialized = isInitialized; }
 
     JSString** singleCharacterStrings() { return &m_singleCharacterStrings[0]; }
 
@@ -127,7 +128,6 @@ private:
     static const unsigned singleCharacterStringCount = maxSingleCharacterString + 1;
 
     void createEmptyString(VM*);
-    void createSingleCharacterString(VM*, unsigned char);
 
     void initialize(VM*, JSString*&, const char* value);
 
@@ -139,8 +139,8 @@ private:
     JSString* m_nullObjectString { nullptr };
     JSString* m_undefinedObjectString { nullptr };
     JSString* m_singleCharacterStrings[singleCharacterStringCount] { nullptr };
-    std::unique_ptr<SmallStringsStorage> m_storage;
     bool m_needsToBeVisited { true };
+    bool m_isInitialized { false };
 };
 
 } // namespace JSC
index 102f98e..9c1e928 100644 (file)
@@ -340,11 +340,14 @@ VM::VM(VMType vmType, HeapType heapType)
     // Need to be careful to keep everything consistent here
     JSLockHolder lock(this);
     AtomicStringTable* existingEntryAtomicStringTable = Thread::current().setCurrentAtomicStringTable(m_atomicStringTable);
-    propertyNames = new CommonIdentifiers(this);
     structureStructure.set(*this, Structure::createStructure(*this));
     structureRareDataStructure.set(*this, StructureRareData::createStructure(*this, 0, jsNull()));
-    terminatedExecutionErrorStructure.set(*this, TerminatedExecutionError::createStructure(*this, 0, jsNull()));
     stringStructure.set(*this, JSString::createStructure(*this, 0, jsNull()));
+
+    smallStrings.initializeCommonStrings(*this);
+
+    propertyNames = new CommonIdentifiers(this);
+    terminatedExecutionErrorStructure.set(*this, TerminatedExecutionError::createStructure(*this, 0, jsNull()));
     propertyNameEnumeratorStructure.set(*this, JSPropertyNameEnumerator::createStructure(*this, 0, jsNull()));
     customGetterSetterStructure.set(*this, CustomGetterSetter::createStructure(*this, 0, jsNull()));
     domAttributeGetterSetterStructure.set(*this, DOMAttributeGetterSetter::createStructure(*this, 0, jsNull()));
@@ -401,8 +404,6 @@ VM::VM(VMType vmType, HeapType heapType)
     sentinelSetBucket.set(*this, JSSet::BucketType::createSentinel(*this));
     sentinelMapBucket.set(*this, JSMap::BucketType::createSentinel(*this));
 
-    smallStrings.initializeCommonStrings(*this);
-
     Thread::current().setCurrentAtomicStringTable(existingEntryAtomicStringTable);
 
 #if ENABLE(JIT)
@@ -539,6 +540,7 @@ VM::~VM()
 
     ASSERT(currentThreadIsHoldingAPILock());
     m_apiLock->willDestroyVM(this);
+    smallStrings.setInitialized(false);
     heap.lastChanceToFinalize();
 
     JSRunLoopTimer::Manager::shared().unregisterVM(*this);