Attachment 341842 Details for Bug 186102 – Patch

[patch] Patch

bug-186102-20180602204017.patch (text/plain), 16.22 KB, created by Yusuke Suzuki on 2018-06-02 04:40:18 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2018-06-02 04:40:18 PDT

Size: 16.22 KB

patch

obsolete

>Subversion Revision: 232434
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 92185c85dc0048570d539a674b2cc31dbe8e33ac..8e4de13565e1460c77da24ededc2d69f2d91be42 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,40 @@
>+2018-06-01  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        [JSC] Remove WeakReferenceHarvester
>+        https://bugs.webkit.org/show_bug.cgi?id=186102
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        After several cleanups, now JSWeakMap becomes the last user of WeakReferenceHarvester.
>+        Since JSWeakMap is already managed in IsoSubspace, we can iterate marked JSWeakMap
>+        by using IsoSubspace::forEachMarkedCell.
>+
>+        This patch removes WeakReferenceHarvester. Instead of managing this linked-list, our
>+        Weak Reference Harvester constraint set iterates marked JSWeakMap by using Subspace.
>+
>+        Attached microbenchmark does not show any regression.
>+
>+        * API/JSAPIWrapperObject.h:
>+        * CMakeLists.txt:
>+        * JavaScriptCore.xcodeproj/project.pbxproj:
>+        * heap/Heap.cpp:
>+        (JSC::Heap::endMarking):
>+        (JSC::Heap::addCoreConstraints):
>+        * heap/Heap.h:
>+        * heap/SlotVisitor.cpp:
>+        (JSC::SlotVisitor::addWeakReferenceHarvester): Deleted.
>+        * heap/SlotVisitor.h:
>+        * heap/WeakReferenceHarvester.h: Removed.
>+        * runtime/WeakMapImpl.cpp:
>+        (JSC::WeakMapImpl<WeakMapBucket>::visitChildren):
>+        (JSC::WeakMapImpl<WeakMapBucket<WeakMapBucketDataKey>>::visitOutputConstraints):
>+        (JSC::WeakMapImpl<WeakMapBucket<WeakMapBucketDataKeyValue>>::visitOutputConstraints):
>+        (JSC::WeakMapImpl<WeakMapBucket<WeakMapBucketDataKey>>::visitWeakReferences): Deleted.
>+        (JSC::WeakMapImpl<WeakMapBucket<WeakMapBucketDataKeyValue>>::visitWeakReferences): Deleted.
>+        * runtime/WeakMapImpl.h:
>+        (JSC::WeakMapImpl::DeadKeyCleaner::target): Deleted.
>+        (): Deleted.
>+
> 2018-06-01  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         Fix the watchOS build after r232385
>diff --git a/Source/JavaScriptCore/API/JSAPIWrapperObject.h b/Source/JavaScriptCore/API/JSAPIWrapperObject.h
>index aae5d418a7a32915ca43ec9728f6e80c2a7e7bbe..305fca4ce0b8fe21d6b8384f9627b367905542d9 100644
>--- a/Source/JavaScriptCore/API/JSAPIWrapperObject.h
>+++ b/Source/JavaScriptCore/API/JSAPIWrapperObject.h
>@@ -29,7 +29,6 @@
> #include "JSBase.h"
> #include "JSCPoison.h"
> #include "JSDestructibleObject.h"
>-#include "WeakReferenceHarvester.h"
> #include <wtf/Poisoned.h>
> 
> #if JSC_OBJC_API_ENABLED || defined(JSC_GLIB_API_ENABLED)
>diff --git a/Source/JavaScriptCore/CMakeLists.txt b/Source/JavaScriptCore/CMakeLists.txt
>index d87bb0309088c22a28fb4939660709305c0a7471..cdecabd1d6b25ba6ec73ab82a2043e0fc260e54a 100644
>--- a/Source/JavaScriptCore/CMakeLists.txt
>+++ b/Source/JavaScriptCore/CMakeLists.txt
>@@ -567,7 +567,6 @@ set(JavaScriptCore_PRIVATE_FRAMEWORK_HEADERS
>     heap/WeakHandleOwner.h
>     heap/WeakImpl.h
>     heap/WeakInlines.h
>-    heap/WeakReferenceHarvester.h
>     heap/WeakSet.h
>     heap/WeakSetInlines.h
> 
>diff --git a/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj b/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
>index 13dafbbd1be5c45fd5eaa11bc4139bf8178b40fd..63e5a14f6a358e831dde020878adaab0c98722bd 100644
>--- a/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
>+++ b/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
>@@ -150,7 +150,6 @@
> 		0F235BE217178E1C00690C7F /* FTLThunks.h in Headers */ = {isa = PBXBuildFile; fileRef = 0F235BCC17178E1C00690C7F /* FTLThunks.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 		0F235BEC17178E7300690C7F /* DFGOSRExitBase.h in Headers */ = {isa = PBXBuildFile; fileRef = 0F235BE817178E7300690C7F /* DFGOSRExitBase.h */; };
> 		0F235BEE17178E7300690C7F /* DFGOSRExitPreparation.h in Headers */ = {isa = PBXBuildFile; fileRef = 0F235BEA17178E7300690C7F /* DFGOSRExitPreparation.h */; };
>-		0F242DA713F3B1E8007ADD4C /* WeakReferenceHarvester.h in Headers */ = {isa = PBXBuildFile; fileRef = 0F242DA513F3B1BB007ADD4C /* WeakReferenceHarvester.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 		0F24E54117EA9F5900ABB217 /* AssemblyHelpers.h in Headers */ = {isa = PBXBuildFile; fileRef = 0F24E53C17EA9F5900ABB217 /* AssemblyHelpers.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 		0F24E54217EA9F5900ABB217 /* CCallHelpers.h in Headers */ = {isa = PBXBuildFile; fileRef = 0F24E53D17EA9F5900ABB217 /* CCallHelpers.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 		0F24E54317EA9F5900ABB217 /* FPRInfo.h in Headers */ = {isa = PBXBuildFile; fileRef = 0F24E53E17EA9F5900ABB217 /* FPRInfo.h */; settings = {ATTRIBUTES = (Private, ); }; };
>@@ -2063,7 +2062,6 @@
> 		0F235BE817178E7300690C7F /* DFGOSRExitBase.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = DFGOSRExitBase.h; path = dfg/DFGOSRExitBase.h; sourceTree = "<group>"; };
> 		0F235BE917178E7300690C7F /* DFGOSRExitPreparation.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = DFGOSRExitPreparation.cpp; path = dfg/DFGOSRExitPreparation.cpp; sourceTree = "<group>"; };
> 		0F235BEA17178E7300690C7F /* DFGOSRExitPreparation.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = DFGOSRExitPreparation.h; path = dfg/DFGOSRExitPreparation.h; sourceTree = "<group>"; };
>-		0F242DA513F3B1BB007ADD4C /* WeakReferenceHarvester.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WeakReferenceHarvester.h; sourceTree = "<group>"; };
> 		0F24E53B17EA9F5900ABB217 /* AssemblyHelpers.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = AssemblyHelpers.cpp; sourceTree = "<group>"; };
> 		0F24E53C17EA9F5900ABB217 /* AssemblyHelpers.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = AssemblyHelpers.h; sourceTree = "<group>"; };
> 		0F24E53D17EA9F5900ABB217 /* CCallHelpers.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CCallHelpers.h; sourceTree = "<group>"; };
>@@ -5821,7 +5819,6 @@
> 				14F7256414EE265E00B1652B /* WeakHandleOwner.h */,
> 				14E84F9D14EE1ACC00D6D5D4 /* WeakImpl.h */,
> 				14BE7D3217135CF400D1807A /* WeakInlines.h */,
>-				0F242DA513F3B1BB007ADD4C /* WeakReferenceHarvester.h */,
> 				14E84F9B14EE1ACC00D6D5D4 /* WeakSet.cpp */,
> 				14E84F9C14EE1ACC00D6D5D4 /* WeakSet.h */,
> 				14150132154BB13F005D8C98 /* WeakSetInlines.h */,
>@@ -9627,7 +9624,6 @@
> 				E3A32BC71FC83147007D7E76 /* WeakMapImpl.h in Headers */,
> 				E393ADD81FE702D00022D681 /* WeakMapImplInlines.h in Headers */,
> 				A7CA3AE617DA41AE006538AF /* WeakMapPrototype.h in Headers */,
>-				0F242DA713F3B1E8007ADD4C /* WeakReferenceHarvester.h in Headers */,
> 				14E84FA114EE1ACC00D6D5D4 /* WeakSet.h in Headers */,
> 				709FB86A1AE335C60039D069 /* WeakSetConstructor.h in Headers */,
> 				14150133154BB13F005D8C98 /* WeakSetInlines.h in Headers */,
>diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp
>index 3b1aacfc3eaa6a6a75a521c5810d9dd94c5c41cb..9c66bbc053e73bd66f452157474fc03659ba9a8f 100644
>--- a/Source/JavaScriptCore/heap/Heap.cpp
>+++ b/Source/JavaScriptCore/heap/Heap.cpp
>@@ -782,7 +782,6 @@ void Heap::endMarking()
>         });
> 
>     assertMarkStacksEmpty();
>-    m_weakReferenceHarvesters.removeAll();
> 
>     RELEASE_ASSERT(m_raceMarkStack->isEmpty());
>     
>@@ -2716,14 +2715,6 @@ void Heap::addCoreConstraints()
>         },
>         ConstraintVolatility::GreyedByMarking);
>     
>-    m_constraintSet->add(
>-        "Wrh", "Weak Reference Harvesters",
>-        [this] (SlotVisitor& slotVisitor) {
>-            for (WeakReferenceHarvester* current = m_weakReferenceHarvesters.head(); current; current = current->next())
>-                current->visitWeakReferences(slotVisitor);
>-        },
>-        ConstraintVolatility::GreyedByMarking);
>-    
>     m_constraintSet->add(
>         "O", "Output",
>         [] (SlotVisitor& slotVisitor) {
>@@ -2740,6 +2731,7 @@ void Heap::addCoreConstraints()
>             };
>             
>             add(vm.executableToCodeBlockEdgesWithConstraints);
>+            add(vm.weakMapSpace);
>         },
>         ConstraintVolatility::GreyedByMarking,
>         ConstraintParallelism::Parallel);
>diff --git a/Source/JavaScriptCore/heap/Heap.h b/Source/JavaScriptCore/heap/Heap.h
>index e7d99fb2b7817ffb03b692c49e3eac12df3381bb..3efd201871351bf5d1fafcabda112bf8d2746989 100644
>--- a/Source/JavaScriptCore/heap/Heap.h
>+++ b/Source/JavaScriptCore/heap/Heap.h
>@@ -40,7 +40,6 @@
> #include "StructureIDTable.h"
> #include "Synchronousness.h"
> #include "WeakHandleOwner.h"
>-#include "WeakReferenceHarvester.h"
> #include <wtf/AutomaticThread.h>
> #include <wtf/ConcurrentPtrHashSet.h>
> #include <wtf/Deque.h>
>@@ -674,8 +673,6 @@ class Heap {
> 
>     static const size_t s_blockFragmentLength = 32;
> 
>-    ListableHandler<WeakReferenceHarvester>::List m_weakReferenceHarvesters;
>-
>     ParallelHelperClient m_helperClient;
>     RefPtr<SharedTask<void(SlotVisitor&)>> m_bonusVisitorTask;
> 
>diff --git a/Source/JavaScriptCore/heap/SlotVisitor.cpp b/Source/JavaScriptCore/heap/SlotVisitor.cpp
>index 5b0924a846d2ca12344fc537a27fec20590549d4..b89686e545a8e70b289d1b421868e9a82dfd7060 100644
>--- a/Source/JavaScriptCore/heap/SlotVisitor.cpp
>+++ b/Source/JavaScriptCore/heap/SlotVisitor.cpp
>@@ -753,11 +753,6 @@ void SlotVisitor::donateAndDrain(MonotonicTime timeout)
>     drain(timeout);
> }
> 
>-void SlotVisitor::addWeakReferenceHarvester(WeakReferenceHarvester* weakReferenceHarvester)
>-{
>-    m_heap.m_weakReferenceHarvesters.addThreadSafe(weakReferenceHarvester);
>-}
>-
> void SlotVisitor::didRace(const VisitRaceKey& race)
> {
>     if (Options::verboseVisitRace())
>diff --git a/Source/JavaScriptCore/heap/SlotVisitor.h b/Source/JavaScriptCore/heap/SlotVisitor.h
>index 29fbfacb56234209331bdbf606dfd15c276cc303..e8a27743a6c63b12ad99773abd260d839ad31731 100644
>--- a/Source/JavaScriptCore/heap/SlotVisitor.h
>+++ b/Source/JavaScriptCore/heap/SlotVisitor.h
>@@ -45,7 +45,6 @@ class MarkedBlock;
> class MarkingConstraint;
> class MarkingConstraintSolver;
> template<typename T> class Weak;
>-class WeakReferenceHarvester;
> template<typename T, typename Traits> class WriteBarrierBase;
> 
> typedef uint32_t HeapVersion;
>@@ -140,8 +139,6 @@ class SlotVisitor {
>     void reportExternalMemoryVisited(size_t);
> #endif
>     
>-    void addWeakReferenceHarvester(WeakReferenceHarvester*);
>-
>     void dump(PrintStream&) const;
> 
>     bool isBuildingHeapSnapshot() const { return !!m_heapSnapshotBuilder; }
>diff --git a/Source/JavaScriptCore/heap/WeakReferenceHarvester.h b/Source/JavaScriptCore/heap/WeakReferenceHarvester.h
>deleted file mode 100644
>index 4807e06791ea7a87a6c96e9209b65f00317a7e05..0000000000000000000000000000000000000000
>--- a/Source/JavaScriptCore/heap/WeakReferenceHarvester.h
>+++ /dev/null
>@@ -1,40 +0,0 @@
>-/*
>- *  Copyright (C) 2011 Apple Inc. All rights reserved.
>- *
>- *  This library is free software; you can redistribute it and/or
>- *  modify it under the terms of the GNU Lesser General Public
>- *  License as published by the Free Software Foundation; either
>- *  version 2 of the License, or (at your option) any later version.
>- *
>- *  This library is distributed in the hope that it will be useful,
>- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>- *  Lesser General Public License for more details.
>- *
>- *  You should have received a copy of the GNU Lesser General Public
>- *  License along with this library; if not, write to the Free Software
>- *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
>- *
>- */
>-
>-#pragma once
>-
>-#include "ListableHandler.h"
>-
>-namespace JSC {
>-
>-class SlotVisitor;
>-
>-class WeakReferenceHarvester : public ListableHandler<WeakReferenceHarvester> {
>-public:
>-    virtual void visitWeakReferences(SlotVisitor&) = 0;
>-    
>-protected:
>-    WeakReferenceHarvester()
>-    {
>-    }
>-    
>-    virtual ~WeakReferenceHarvester() { }
>-};
>-
>-} // namespace JSC
>diff --git a/Source/JavaScriptCore/runtime/WeakMapImpl.cpp b/Source/JavaScriptCore/runtime/WeakMapImpl.cpp
>index fc5ebdcc4fa0269223d664478b712232e6c21ab4..c4a360febfcec0474a072bbb68841c7743cdd851 100644
>--- a/Source/JavaScriptCore/runtime/WeakMapImpl.cpp
>+++ b/Source/JavaScriptCore/runtime/WeakMapImpl.cpp
>@@ -45,9 +45,6 @@ void WeakMapImpl<WeakMapBucket>::visitChildren(JSCell* cell, SlotVisitor& visito
>     WeakMapImpl* thisObject = jsCast<WeakMapImpl*>(cell);
>     ASSERT_GC_OBJECT_INHERITS(thisObject, info());
>     Base::visitChildren(thisObject, visitor);
>-
>-    if (isWeakMap())
>-        visitor.addWeakReferenceHarvester(&thisObject->m_deadKeyCleaner);
>     visitor.reportExtraMemoryVisited(thisObject->m_capacity * sizeof(WeakMapBucket));
> }
> 
>@@ -59,16 +56,17 @@ size_t WeakMapImpl<WeakMapBucket>::estimatedSize(JSCell* cell)
> }
> 
> template <>
>-void WeakMapImpl<WeakMapBucket<WeakMapBucketDataKey>>::visitWeakReferences(SlotVisitor&)
>+void WeakMapImpl<WeakMapBucket<WeakMapBucketDataKey>>::visitOutputConstraints(JSCell*, SlotVisitor&)
> {
>     // Only JSWeakMap needs to harvest value references
> }
> 
> template <>
>-void WeakMapImpl<WeakMapBucket<WeakMapBucketDataKeyValue>>::visitWeakReferences(SlotVisitor& visitor)
>+void WeakMapImpl<WeakMapBucket<WeakMapBucketDataKeyValue>>::visitOutputConstraints(JSCell* cell, SlotVisitor& visitor)
> {
>-    auto* buffer = this->buffer();
>-    for (uint32_t index = 0; index < m_capacity; ++index) {
>+    auto* thisObject = jsCast<WeakMapImpl*>(cell);
>+    auto* buffer = thisObject->buffer();
>+    for (uint32_t index = 0; index < thisObject->m_capacity; ++index) {
>         auto* bucket = buffer + index;
>         if (bucket->isEmpty() || bucket->isDeleted())
>             continue;
>diff --git a/Source/JavaScriptCore/runtime/WeakMapImpl.h b/Source/JavaScriptCore/runtime/WeakMapImpl.h
>index 1bf6350b08cdd106c7a8b6aee19658e53e879663..e29b329f67c3ddbb6f3979851c0afa11bcfdb9b7 100644
>--- a/Source/JavaScriptCore/runtime/WeakMapImpl.h
>+++ b/Source/JavaScriptCore/runtime/WeakMapImpl.h
>@@ -309,6 +309,7 @@ class WeakMapImpl : public JSDestructibleObject {
>         return &vm.weakSetSpace;
>     }
> 
>+    static void visitOutputConstraints(JSCell*, SlotVisitor&);
>     void finalizeUnconditionally(VM&);
> 
> private:
>@@ -469,27 +470,10 @@ class WeakMapImpl : public JSDestructibleObject {
>     template<typename Appender>
>     void takeSnapshotInternal(unsigned limit, Appender);
> 
>-    class DeadKeyCleaner : public WeakReferenceHarvester {
>-    public:
>-        WeakMapImpl* target()
>-        {
>-            return bitwise_cast<WeakMapImpl*>(bitwise_cast<char*>(this) - OBJECT_OFFSETOF(WeakMapImpl, m_deadKeyCleaner));
>-        }
>-
>-    private:
>-        void visitWeakReferences(SlotVisitor& visitor) override
>-        {
>-            target()->visitWeakReferences(visitor);
>-        }
>-    };
>-
>-    void visitWeakReferences(SlotVisitor&);
>-
>     MallocPtr<WeakMapBufferType, JSValueMalloc> m_buffer;
>     uint32_t m_keyCount;
>     uint32_t m_deleteCount;
>     uint32_t m_capacity;
>-    DeadKeyCleaner m_deadKeyCleaner;
> };
> 
> } // namespace JSC
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index e37f8dbde18a69a059aa8c937fa29589ecfd92e4..5d7ed997289376da9f0279a2e8f64d89c69c939d 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,12 @@
>+2018-06-01  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        [JSC] Remove WeakReferenceHarvester
>+        https://bugs.webkit.org/show_bug.cgi?id=186102
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * microbenchmarks/create-many-weak-map.js: Added.
>+
> 2018-05-29  Yusuke Suzuki  <utatane.tea@gmail.com>
> 
>         [JSC] Add Symbol.prototype.description getter
>diff --git a/JSTests/microbenchmarks/create-many-weak-map.js b/JSTests/microbenchmarks/create-many-weak-map.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..1d30d123b41b6f67600ec3b17e27821c42f3dc96
>--- /dev/null
>+++ b/JSTests/microbenchmarks/create-many-weak-map.js
>@@ -0,0 +1,20 @@
>+var root = [];
>+
>+function create()
>+{
>+    var weakMap = new WeakMap();
>+    for (var i = 0; i < 1e2; ++i) {
>+        var key = {};
>+        var value = {};
>+        weakMap.set(key, value);
>+        if (i % 10 === 0)
>+            root.push(key);
>+    }
>+}
>+
>+for (var i = 0; i < 1e2; ++i) {
>+    for (var j = 0; j < 1e2; ++j) {
>+        var map = create();
>+        root.push(map);
>+    }
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 186102: 341582 | 341583 | 341842 | 341844