PerProcess<> should be safe by default
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 11 Mar 2018 17:45:49 +0000 (17:45 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 11 Mar 2018 17:45:49 +0000 (17:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183545

Reviewed by Yusuke Suzuki.

This makes PerProcess<> safe by default, so we don't need SafePerProcess<>.

The new PerProcess<> design relies on a hash-consing mechanism for PerProcess<> storage based
on the __PRETTY_FUNCTION__ from inside PerProcess<>, which captures the instantiated type in
the string. Therefore, this can be used to runtime-coalesce PerProcess<> instances based on
type.

I expect this to be perf-neutral. It's an important prerequisite to more bmalloc work, since I
don't want to have more PerProcess<> vs SafePerProcess<> bugs, and SafePerProcess<> can't be
used for everything (I don't see how to use it for isoheaps).

* CMakeLists.txt:
* bmalloc.xcodeproj/project.pbxproj:
* bmalloc/Heap.cpp:
(bmalloc::Heap::Heap):
* bmalloc/IsoDirectoryInlines.h:
(bmalloc::passedNumPages>::takeFirstEligible):
(bmalloc::passedNumPages>::didBecome):
* bmalloc/PerProcess.cpp: Added.
(bmalloc::stringHash):
(bmalloc::allocate):
(bmalloc::getPerProcessData):
* bmalloc/PerProcess.h:
(bmalloc::PerProcess::mutex):
(bmalloc::PerProcess::coalesce):
(bmalloc::PerProcess::getSlowCase):
(): Deleted.
* bmalloc/Scavenger.cpp:
* bmalloc/Scavenger.h:
* bmalloc/bmalloc.cpp:
(bmalloc::api::scavenge):
(bmalloc::api::setScavengerThreadQOSClass):

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

Source/bmalloc/CMakeLists.txt
Source/bmalloc/ChangeLog
Source/bmalloc/bmalloc.xcodeproj/project.pbxproj
Source/bmalloc/bmalloc/CryptoRandom.cpp
Source/bmalloc/bmalloc/Heap.cpp
Source/bmalloc/bmalloc/IsoDirectoryInlines.h
Source/bmalloc/bmalloc/PerProcess.cpp [new file with mode: 0644]
Source/bmalloc/bmalloc/PerProcess.h
Source/bmalloc/bmalloc/Scavenger.cpp
Source/bmalloc/bmalloc/Scavenger.h
Source/bmalloc/bmalloc/bmalloc.cpp

index 51c388e..a1649ab 100644 (file)
@@ -25,6 +25,7 @@ set(bmalloc_SOURCES
     bmalloc/LargeMap.cpp
     bmalloc/Logging.cpp
     bmalloc/ObjectType.cpp
+    bmalloc/PerProcess.cpp
     bmalloc/Scavenger.cpp
     bmalloc/StaticMutex.cpp
     bmalloc/VMHeap.cpp
index de6a069..13cad61 100644 (file)
@@ -1,3 +1,43 @@
+2018-03-10  Filip Pizlo  <fpizlo@apple.com>
+
+        PerProcess<> should be safe by default
+        https://bugs.webkit.org/show_bug.cgi?id=183545
+
+        Reviewed by Yusuke Suzuki.
+        
+        This makes PerProcess<> safe by default, so we don't need SafePerProcess<>.
+        
+        The new PerProcess<> design relies on a hash-consing mechanism for PerProcess<> storage based
+        on the __PRETTY_FUNCTION__ from inside PerProcess<>, which captures the instantiated type in
+        the string. Therefore, this can be used to runtime-coalesce PerProcess<> instances based on
+        type.
+        
+        I expect this to be perf-neutral. It's an important prerequisite to more bmalloc work, since I
+        don't want to have more PerProcess<> vs SafePerProcess<> bugs, and SafePerProcess<> can't be
+        used for everything (I don't see how to use it for isoheaps).
+
+        * CMakeLists.txt:
+        * bmalloc.xcodeproj/project.pbxproj:
+        * bmalloc/Heap.cpp:
+        (bmalloc::Heap::Heap):
+        * bmalloc/IsoDirectoryInlines.h:
+        (bmalloc::passedNumPages>::takeFirstEligible):
+        (bmalloc::passedNumPages>::didBecome):
+        * bmalloc/PerProcess.cpp: Added.
+        (bmalloc::stringHash):
+        (bmalloc::allocate):
+        (bmalloc::getPerProcessData):
+        * bmalloc/PerProcess.h:
+        (bmalloc::PerProcess::mutex):
+        (bmalloc::PerProcess::coalesce):
+        (bmalloc::PerProcess::getSlowCase):
+        (): Deleted.
+        * bmalloc/Scavenger.cpp:
+        * bmalloc/Scavenger.h:
+        * bmalloc/bmalloc.cpp:
+        (bmalloc::api::scavenge):
+        (bmalloc::api::setScavengerThreadQOSClass):
+
 2018-03-10  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r229436.
index 4e80e4f..e32618c 100644 (file)
@@ -21,6 +21,7 @@
 /* End PBXAggregateTarget section */
 
 /* Begin PBXBuildFile section */
+               0F26A7A5205483130090A141 /* PerProcess.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 0F26A7A42054830D0090A141 /* PerProcess.cpp */; };
                0F5167741FAD685C008236A8 /* bmalloc.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 0F5167731FAD6852008236A8 /* bmalloc.cpp */; };
                0F5549EF1FB54704007FF75A /* IsoPage.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 0F5549EE1FB54701007FF75A /* IsoPage.cpp */; };
                0F5BF1471F22A8B10029D91D /* HeapKind.h in Headers */ = {isa = PBXBuildFile; fileRef = 0F5BF1461F22A8B10029D91D /* HeapKind.h */; settings = {ATTRIBUTES = (Private, ); }; };
 /* End PBXCopyFilesBuildPhase section */
 
 /* Begin PBXFileReference section */
+               0F26A7A42054830D0090A141 /* PerProcess.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = PerProcess.cpp; path = bmalloc/PerProcess.cpp; sourceTree = "<group>"; };
                0F5167731FAD6852008236A8 /* bmalloc.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = bmalloc.cpp; path = bmalloc/bmalloc.cpp; sourceTree = "<group>"; };
                0F5549EE1FB54701007FF75A /* IsoPage.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = IsoPage.cpp; path = bmalloc/IsoPage.cpp; sourceTree = "<group>"; };
                0F5BF1461F22A8B10029D91D /* HeapKind.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = HeapKind.h; path = bmalloc/HeapKind.h; sourceTree = "<group>"; };
                                4426E27F1C838EE0008EB042 /* Logging.h */,
                                14C8992A1CC485E70027A057 /* Map.h */,
                                144DCED617A649D90093B2F2 /* Mutex.h */,
+                               0F26A7A42054830D0090A141 /* PerProcess.cpp */,
                                0F5BF1481F22A8D80029D91D /* PerHeapKind.h */,
                                14446A0717A61FA400F9EA1D /* PerProcess.h */,
                                144469FD17A61F1F00F9EA1D /* PerThread.h */,
                                14F271C318EA3978008C152F /* Allocator.cpp in Sources */,
                                6599C5CC1EC3F15900A2F7BB /* AvailableMemory.cpp in Sources */,
                                14F271C418EA397B008C152F /* Cache.cpp in Sources */,
+                               0F26A7A5205483130090A141 /* PerProcess.cpp in Sources */,
                                14F271C518EA397E008C152F /* Deallocator.cpp in Sources */,
                                142B44361E2839E7001DA6E9 /* DebugHeap.cpp in Sources */,
                                14895D911A3A319C0006235D /* Environment.cpp in Sources */,
index f0f41c3..19a565d 100644 (file)
@@ -54,8 +54,6 @@ int CCRandomCopyBytes(CCRandomRef rnd, void *bytes, size_t count);
 
 namespace bmalloc {
 
-namespace {
-
 class ARC4Stream {
 public:
     ARC4Stream();
@@ -180,8 +178,6 @@ void ARC4RandomNumberGenerator::randomValues(void* buffer, size_t length)
     }
 }
 
-}
-
 void cryptoRandom(void* buffer, size_t length)
 {
     PerProcess<ARC4RandomNumberGenerator>::get()->randomValues(buffer, length);
index 9151845..180244f 100644 (file)
@@ -64,7 +64,7 @@ Heap::Heap(HeapKind kind, std::lock_guard<StaticMutex>&)
 #endif
     }
     
-    m_scavenger = SafePerProcess<Scavenger>::get();
+    m_scavenger = PerProcess<Scavenger>::get();
 }
 
 bool Heap::usingGigacage()
index a66ca8f..c98d013 100644 (file)
@@ -51,7 +51,7 @@ EligibilityResult<Config> IsoDirectory<Config, passedNumPages>::takeFirstEligibl
     if (pageIndex >= numPages)
         return EligibilityKind::Full;
     
-    Scavenger& scavenger = *SafePerProcess<Scavenger>::get();
+    Scavenger& scavenger = *PerProcess<Scavenger>::get();
     scavenger.didStartGrowing();
     
     IsoPage<Config>* page = m_pages[pageIndex];
@@ -100,7 +100,7 @@ void IsoDirectory<Config, passedNumPages>::didBecome(IsoPage<Config>* page, IsoP
         if (verbose)
             fprintf(stderr, "%p: %p did become empty.\n", this, page);
         m_empty[pageIndex] = true;
-        SafePerProcess<Scavenger>::get()->schedule(IsoPageBase::pageSize);
+        PerProcess<Scavenger>::get()->schedule(IsoPageBase::pageSize);
         return;
     }
     BCRASH();
diff --git a/Source/bmalloc/bmalloc/PerProcess.cpp b/Source/bmalloc/bmalloc/PerProcess.cpp
new file mode 100644 (file)
index 0000000..ed6f04f
--- /dev/null
@@ -0,0 +1,90 @@
+/*
+ * Copyright (C) 2018 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
+ */
+
+#include "PerProcess.h"
+
+#include "VMAllocate.h"
+
+namespace bmalloc {
+
+static constexpr unsigned tableSize = 100;
+static constexpr bool verbose = false;
+
+static StaticMutex s_mutex;
+
+static char* s_bumpBase;
+static size_t s_bumpOffset;
+static size_t s_bumpLimit;
+
+static PerProcessData* s_table[tableSize];
+
+// Returns zero-filled memory by virtual of using vmAllocate.
+static void* allocate(size_t size, size_t alignment)
+{
+    s_bumpOffset = roundUpToMultipleOf(alignment, s_bumpOffset);
+    void* result = s_bumpBase + s_bumpOffset;
+    s_bumpOffset += size;
+    if (s_bumpOffset <= s_bumpLimit)
+        return result;
+    
+    size_t allocationSize = vmSize(size);
+    void* allocation = vmAllocate(allocationSize);
+    s_bumpBase = static_cast<char*>(allocation);
+    s_bumpOffset = 0;
+    s_bumpLimit = allocationSize;
+    return allocate(size, alignment);
+}
+
+PerProcessData* getPerProcessData(unsigned hash, const char* disambiguator, size_t size, size_t alignment)
+{
+    std::lock_guard<StaticMutex> lock(s_mutex);
+
+    PerProcessData*& bucket = s_table[hash % tableSize];
+    
+    for (PerProcessData* data = bucket; data; data = data->next) {
+        if (!strcmp(data->disambiguator, disambiguator)) {
+            if (verbose)
+                fprintf(stderr, "%d: Using existing %s\n", getpid(), disambiguator);
+            RELEASE_BASSERT(data->size == size);
+            RELEASE_BASSERT(data->alignment == alignment);
+            return data;
+        }
+    }
+    
+    if (verbose)
+        fprintf(stderr, "%d: Creating new %s\n", getpid(), disambiguator);
+    void* rawDataPtr = allocate(sizeof(PerProcessData), std::alignment_of<PerProcessData>::value);
+    PerProcessData* data = static_cast<PerProcessData*>(rawDataPtr);
+    data->disambiguator = disambiguator;
+    data->memory = allocate(size, alignment);
+    data->size = size;
+    data->alignment = alignment;
+    data->next = bucket;
+    bucket = data;
+    return data;
+}
+
+} // namespace bmalloc
+
index 60e279b..2616525 100644 (file)
@@ -38,10 +38,6 @@ namespace bmalloc {
 //
 // Object will be instantiated only once, even in the face of concurrency.
 //
-// WARNING: PerProcess<T> does not export its storage. So in actuality when
-// used in multiple libraries / images it ends up being per-image. To truly
-// declare a per-process singleton use SafePerProcess<T>.
-//
 // NOTE: If you observe global side-effects of the Object constructor, be
 // sure to lock the Object mutex. For example:
 //
@@ -55,6 +51,26 @@ namespace bmalloc {
 // Object* object = PerProcess<Object>::get(lock);
 // if (globalFlag) { ... } // OK.
 
+struct PerProcessData {
+    const char* disambiguator;
+    void* memory;
+    size_t size;
+    size_t alignment;
+    StaticMutex mutex;
+    bool isInitialized;
+    PerProcessData* next;
+};
+
+constexpr unsigned stringHash(const char* string)
+{
+    unsigned result = 5381;
+    while (char c = *string++)
+        result = result * 33 + c;
+    return result;
+}
+
+BEXPORT PerProcessData* getPerProcessData(unsigned disambiguatorHash, const char* disambiguator, size_t size, size_t alignment);
+
 template<typename T>
 class PerProcess {
 public:
@@ -71,88 +87,46 @@ public:
         return s_object.load(std::memory_order_relaxed);
     }
     
-    static StaticMutex& mutex() { return s_mutex; }
+    static StaticMutex& mutex()
+    {
+        if (!s_data)
+            coalesce();
+        return s_data->mutex;
+    }
 
 private:
+    static void coalesce()
+    {
+        if (s_data)
+            return;
+        
+        const char* disambiguator = __PRETTY_FUNCTION__;
+        s_data = getPerProcessData(stringHash(disambiguator), disambiguator, sizeof(T), std::alignment_of<T>::value);
+    }
+    
     BNO_INLINE static T* getSlowCase()
     {
-        std::lock_guard<StaticMutex> lock(s_mutex);
-        if (!s_object.load(std::memory_order_consume)) {
-            T* t = new (&s_memory) T(lock);
-            s_object.store(t, std::memory_order_release);
+        std::lock_guard<StaticMutex> lock(mutex());
+        if (!s_object.load()) {
+            if (s_data->isInitialized)
+                s_object.store(static_cast<T*>(s_data->memory));
+            else {
+                T* t = new (s_data->memory) T(lock);
+                s_object.store(t);
+                s_data->isInitialized = true;
+            }
         }
-        return s_object.load(std::memory_order_consume);
+        return s_object.load();
     }
 
     static std::atomic<T*> s_object;
-    static StaticMutex s_mutex;
-
-    typedef typename std::aligned_storage<sizeof(T), std::alignment_of<T>::value>::type Memory;
-    static Memory s_memory;
+    static PerProcessData* s_data;
 };
 
 template<typename T>
 std::atomic<T*> PerProcess<T>::s_object;
 
 template<typename T>
-StaticMutex PerProcess<T>::s_mutex;
-
-template<typename T>
-typename PerProcess<T>::Memory PerProcess<T>::s_memory;
-
-
-// SafePerProcess<T> behaves like PerProcess<T>, but its usage
-// requires DECLARE/DEFINE macros that export symbols that allow for
-// a single shared instance is across images in the process.
-
-template<typename T> struct SafePerProcessStorageTraits;
-
-template<typename T>
-class BEXPORT SafePerProcess {
-public:
-    using Storage = typename SafePerProcessStorageTraits<T>::Storage;
-
-    static T* get()
-    {
-        T* object = getFastCase();
-        if (!object)
-            return getSlowCase();
-        return object;
-    }
-
-    static T* getFastCase()
-    {
-        return (Storage::s_object).load(std::memory_order_relaxed);
-    }
-
-    static StaticMutex& mutex() { return Storage::s_mutex; }
-
-    using Memory = typename std::aligned_storage<sizeof(T), std::alignment_of<T>::value>::type;
-
-private:
-    BNO_INLINE static T* getSlowCase()
-    {
-        std::lock_guard<StaticMutex> lock(Storage::s_mutex);
-        if (!Storage::s_object.load(std::memory_order_consume)) {
-            T* t = new (&Storage::s_memory) T(lock);
-            Storage::s_object.store(t, std::memory_order_release);
-        }
-        return Storage::s_object.load(std::memory_order_consume);
-    }
-};
-
-#define DECLARE_SAFE_PER_PROCESS_STORAGE(Type) \
-    template<> struct SafePerProcessStorageTraits<Type> { \
-        struct BEXPORT Storage { \
-            BEXPORT static std::atomic<Type*> s_object; \
-            BEXPORT static StaticMutex s_mutex; \
-            BEXPORT static SafePerProcess<Type>::Memory s_memory; \
-        }; \
-    };
-
-#define DEFINE_SAFE_PER_PROCESS_STORAGE(Type) \
-    std::atomic<Type*> SafePerProcessStorageTraits<Type>::Storage::s_object; \
-    StaticMutex SafePerProcessStorageTraits<Type>::Storage::s_mutex; \
-    SafePerProcess<Type>::Memory SafePerProcessStorageTraits<Type>::Storage::s_memory;
+PerProcessData* PerProcess<T>::s_data;
 
 } // namespace bmalloc
index 0bdc77d..030cfcb 100644 (file)
@@ -32,8 +32,6 @@
 
 namespace bmalloc {
 
-DEFINE_SAFE_PER_PROCESS_STORAGE(Scavenger);
-
 Scavenger::Scavenger(std::lock_guard<StaticMutex>&)
 {
 #if BOS(DARWIN)
index 15b37df..a5df58d 100644 (file)
@@ -93,8 +93,6 @@ private:
     Vector<DeferredDecommit> m_deferredDecommits;
 };
 
-DECLARE_SAFE_PER_PROCESS_STORAGE(Scavenger);
-
 } // namespace bmalloc
 
 
index 6139bc8..4e4b824 100644 (file)
@@ -73,7 +73,7 @@ void scavenge()
 {
     scavengeThisThread();
 
-    SafePerProcess<Scavenger>::get()->scavenge();
+    PerProcess<Scavenger>::get()->scavenge();
 }
 
 bool isEnabled(HeapKind kind)
@@ -87,7 +87,7 @@ bool isEnabled(HeapKind kind)
 void setScavengerThreadQOSClass(qos_class_t overrideClass)
 {
     std::unique_lock<StaticMutex> lock(Heap::mutex());
-    SafePerProcess<Scavenger>::get()->setScavengerThreadQOSClass(overrideClass);
+    PerProcess<Scavenger>::get()->setScavengerThreadQOSClass(overrideClass);
 }
 #endif