Merge r241954,r241955 - [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):
WebKit · Mar 5, 2019 · ac6f53f · ac6f53f
1 parent 87ff66e
commit ac6f53f
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 47 deletions.
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,43 @@
+2019-02-22  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        Unreviewed, build fix after r241954
+        https://bugs.webkit.org/show_bug.cgi?id=194939
+
+        Renaming setCanAccessHeap was incomplete.
+
+        * runtime/SmallStrings.cpp:
+        (JSC::SmallStrings::initializeCommonStrings):
+        * runtime/VM.cpp:
+        (JSC::VM::~VM):
+
+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-21  Mark Lam  <mark.lam@apple.com>
 
         Add more doesGC() assertions.

diff --git a/Source/JavaScriptCore/runtime/SmallStrings.cpp b/Source/JavaScriptCore/runtime/SmallStrings.cpp
@@ -34,30 +34,6 @@
 
 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]");
+
+    setIsInitialized(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)

diff --git a/Source/JavaScriptCore/runtime/SmallStrings.h b/Source/JavaScriptCore/runtime/SmallStrings.h
@@ -51,7 +51,6 @@ namespace JSC {
 
 class VM;
 class JSString;
-class SmallStringsStorage;
 class SlotVisitor;
 
 static const unsigned maxSingleCharacterString = 0xFF;
@@ -72,7 +71,9 @@ class SmallStrings {
         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 @@ class SmallStrings {
     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 @@ class SmallStrings {
     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
diff --git a/Source/JavaScriptCore/runtime/VM.cpp b/Source/JavaScriptCore/runtime/VM.cpp
@@ -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.setIsInitialized(false);
     heap.lastChanceToFinalize();
 
     JSRunLoopTimer::Manager::shared().unregisterVM(*this);