Changeset 241954 in webkit

Timestamp:

Feb 22, 2019 11:04:40 AM (5 years ago)

Author:

ysuzuki@apple.com

Message:

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

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• runtime/SmallStrings.cpp (modified) (4 diffs)
• runtime/SmallStrings.h (modified) (4 diffs)
• runtime/VM.cpp (modified) (3 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/runtime/SmallStrings.cpp

@@ -35 +35 @@
 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()
 {
@@ -70 +46 @@
 {
     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)
@@ -78 +60 @@
     initialize(&vm, m_nullObjectString, "[object Null]");
     initialize(&vm, m_undefinedObjectString, "[object Undefined]");
+
+    setInitialized(true);
 }
 
@@ -105 +89 @@
 }
 
-void SmallStrings::createSingleCharacterString(VM* vm, unsigned char character)
+Ref<StringImpl> SmallStrings::singleCharacterStringRep(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)
-{
-    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();
 }
 

trunk/Source/JavaScriptCore/runtime/SmallStrings.h

@@ -52 +52 @@
 class VM;
 class JSString;
-class SmallStringsStorage;
 class SlotVisitor;
 
@@ -73 +72 @@
     }
 
-    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]; }
@@ -128 +129 @@
 
     void createEmptyString(VM*);
-    void createSingleCharacterString(VM*, unsigned char);
 
     void initialize(VM*, JSString*&, const char* value);
@@ -140 +140 @@
     JSString* m_undefinedObjectString { nullptr };
     JSString* m_singleCharacterStrings[singleCharacterStringCount] { nullptr };
-    std::unique_ptr<SmallStringsStorage> m_storage;
     bool m_needsToBeVisited { true };
+    bool m_isInitialized { false };
 };
 

trunk/Source/JavaScriptCore/runtime/VM.cpp

@@ -341 +341 @@
     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()));
+    stringStructure.set(*this, JSString::createStructure(*this, 0, jsNull()));
+
+    smallStrings.initializeCommonStrings(*this);
+
+    propertyNames = new CommonIdentifiers(this);
     terminatedExecutionErrorStructure.set(*this, TerminatedExecutionError::createStructure(*this, 0, jsNull()));
-    stringStructure.set(*this, JSString::createStructure(*this, 0, jsNull()));
     propertyNameEnumeratorStructure.set(*this, JSPropertyNameEnumerator::createStructure(*this, 0, jsNull()));
     customGetterSetterStructure.set(*this, CustomGetterSetter::createStructure(*this, 0, jsNull()));
@@ -402 +405 @@
     sentinelMapBucket.set(*this, JSMap::BucketType::createSentinel(*this));
 
-    smallStrings.initializeCommonStrings(*this);
-
     Thread::current().setCurrentAtomicStringTable(existingEntryAtomicStringTable);
 
@@ -540 +541 @@
     ASSERT(currentThreadIsHoldingAPILock());
     m_apiLock->willDestroyVM(this);
+    smallStrings.setInitialized(false);
     heap.lastChanceToFinalize();