Attachment 362711 Details for Bug 194939 – Patch

[patch] Patch

bug-194939-20190222021028.patch (text/plain), 8.57 KB, created by Yusuke Suzuki on 2019-02-22 02:10:29 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-02-22 02:10:29 PST

Size: 8.57 KB

patch

obsolete

>Subversion Revision: 241938
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index fc61627c0445961b84d3e25df313b9656624588e..3e1d116608ec98dd46730fd0732c0240fb850495 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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
>diff --git a/Source/JavaScriptCore/runtime/SmallStrings.cpp b/Source/JavaScriptCore/runtime/SmallStrings.cpp
>index 8f26b3e1e2a164e7f3c3347287c0f4e33734747f..709846dfe316ee640c252f92fba347adba2356c4 100644
>--- 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,8 +45,15 @@ 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);
>+    }
>+    setCanAccessHeap(true);
>+
> #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
>@@ -104,20 +87,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_canAccessHeap))
>+        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
>index 769d494f320c403eff6f1539e22396f09cf83486..0f220aede7a1ec586e52b6aca5cae8f9cd65fffc 100644
>--- 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 setCanAccessHeap(bool canAccessHeap) { m_canAccessHeap = canAccessHeap; }
> 
>     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_canAccessHeap { false };
> };
> 
> } // namespace JSC
>diff --git a/Source/JavaScriptCore/runtime/VM.cpp b/Source/JavaScriptCore/runtime/VM.cpp
>index 102f98effdc49e889d7ca9aa19fc000857a61d1d..d010e5eb3ca588a05a1ee4650cf421c766d8666b 100644
>--- 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.setCanAccessHeap(false);
>     heap.lastChanceToFinalize();
> 
>     JSRunLoopTimer::Manager::shared().unregisterVM(*this);

Flags:

mark.lam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 194939: 362709 | 362710 | 362711