WebKit Bugzilla
Attachment 362709 Details for
Bug 194939
: [JSC] SmallStringsStorage is unnecessary
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-194939-20190222015422.patch (text/plain), 8.33 KB, created by
Yusuke Suzuki
on 2019-02-22 01:54:23 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Yusuke Suzuki
Created:
2019-02-22 01:54:23 PST
Size:
8.33 KB
patch
obsolete
>Subversion Revision: 241938 >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index fc61627c0445961b84d3e25df313b9656624588e..b4f408e36ea27a1f60aa545b98ebbc59560aacb9 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,30 @@ >+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_finalized flag to SmallStrings. At the time of VM destruction, JSStrings are destroyed when >+ VM's Heap is finalized. We add this boolean flag to allow users to get StringImpl during the period between >+ VM's Heap destruction and completion of VM destruction. >+ >+ * 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::finalize): >+ * 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..a0cec04305b8a855febb8d0849842727aa8848ee 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,14 @@ 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(StringImpl::create(string, 1).ptr())); >+ 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 >@@ -104,20 +86,13 @@ void SmallStrings::createEmptyString(VM* vm) > ASSERT(m_needsToBeVisited); > } > >-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 (UNLIKELY(m_finalized)) { >+ const LChar string[] = { static_cast<LChar>(character) }; >+ return AtomicStringImpl::add(string, 1).releaseNonNull(); >+ } >+ return *const_cast<StringImpl*>(m_singleCharacterStrings[character]->tryGetValueImpl()); > } > > 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..8358461674b7b837379f19b78cab1f2378e93f67 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 finalize() { m_finalized = true; } > > 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_finalized { false }; > }; > > } // namespace JSC >diff --git a/Source/JavaScriptCore/runtime/VM.cpp b/Source/JavaScriptCore/runtime/VM.cpp >index 102f98effdc49e889d7ca9aa19fc000857a61d1d..24c3c210ea5d77f92c6d78fbd017b6d8160f2350 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.finalize(); > heap.lastChanceToFinalize(); > > JSRunLoopTimer::Manager::shared().unregisterVM(*this);
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 194939
:
362709
|
362710
|
362711