Changeset 279861 in webkit

Timestamp:

Jul 12, 2021 6:47:47 PM (3 years ago)

Author:

mark.lam@apple.com

Message:

Revert r277027: breaks GC.
​https://bugs.webkit.org/show_bug.cgi?id=227888

Reviewed by Saam Barati.

The patch in r277027 to make deletion of GCAwareJITStubRoutines incremental has a
bug: the routine may not be deleted yet by the incremental sweeper before the next
GC cycle, and the GC will not be happy visiting dead cell pointers in that routine.
There is also another bug with the triggering of sweeping.

For now, we're reverting the patch, and will revisit this at a later time.

• CMakeLists.txt:
• heap/Heap.cpp:

(JSC::Heap::deleteUnmarkedCompiledCode):
(JSC::Heap::sweepSynchronously):

• heap/Heap.h:
• heap/HeapInlines.h:

(JSC::Heap::mayHaveJITStubRoutinesToDelete): Deleted.
(JSC::Heap::deleteDeadJITStubRoutines): Deleted.

• heap/IncrementalSweeper.cpp:

(JSC::IncrementalSweeper::doSweep):

• heap/JITStubRoutineSet.cpp:

(JSC::JITStubRoutineSet::deleteUnmarkedJettisonedStubRoutines):

• heap/JITStubRoutineSet.h:

(JSC::JITStubRoutineSet::deleteUnmarkedJettisonedStubRoutines):
(JSC::JITStubRoutineSet::traceMarkedStubRoutines):
(JSC::JITStubRoutineSet::mayHaveRoutinesToDelete): Deleted.
(JSC::JITStubRoutineSet::notifyHaveRoutinesToDelete): Deleted.

• jit/GCAwareJITStubRoutine.cpp:

(JSC::GCAwareJITStubRoutine::observeZeroRefCount):

• jit/JITStubRoutine.h:

(JSC::JITStubRoutine::createSelfManagedRoutine):

Location:

trunk/Source/JavaScriptCore

Files:

• CMakeLists.txt (modified) (1 diff)
• ChangeLog (modified) (1 diff)
• heap/Heap.cpp (modified) (2 diffs)
• heap/Heap.h (modified) (1 diff)
• heap/HeapInlines.h (modified) (2 diffs)
• heap/IncrementalSweeper.cpp (modified) (2 diffs)
• heap/JITStubRoutineSet.cpp (modified) (2 diffs)
• heap/JITStubRoutineSet.h (modified) (4 diffs)
• jit/GCAwareJITStubRoutine.cpp (modified) (1 diff)
• jit/JITStubRoutine.h (modified) (2 diffs)

trunk/Source/JavaScriptCore/CMakeLists.txt

@@ -671 +671 @@
     heap/IsoSubspaceInlines.h
     heap/IsoSubspacePerVM.h
-    heap/JITStubRoutineSet.h
     heap/LocalAllocator.h
     heap/LocalAllocatorInlines.h

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2021-07-12  Mark Lam  <mark.lam@apple.com>
+
+        Revert r277027: breaks GC.
+        https://bugs.webkit.org/show_bug.cgi?id=227888
+
+        Reviewed by Saam Barati.
+
+        The patch in r277027 to make deletion of GCAwareJITStubRoutines incremental has a
+        bug: the routine may not be deleted yet by the incremental sweeper before the next
+        GC cycle, and the GC will not be happy visiting dead cell pointers in that routine.
+        There is also another bug with the triggering of sweeping.
+
+        For now, we're reverting the patch, and will revisit this at a later time.
+
+        * CMakeLists.txt:
+        * heap/Heap.cpp:
+        (JSC::Heap::deleteUnmarkedCompiledCode):
+        (JSC::Heap::sweepSynchronously):
+        * heap/Heap.h:
+        * heap/HeapInlines.h:
+        (JSC::Heap::mayHaveJITStubRoutinesToDelete): Deleted.
+        (JSC::Heap::deleteDeadJITStubRoutines): Deleted.
+        * heap/IncrementalSweeper.cpp:
+        (JSC::IncrementalSweeper::doSweep):
+        * heap/JITStubRoutineSet.cpp:
+        (JSC::JITStubRoutineSet::deleteUnmarkedJettisonedStubRoutines):
+        * heap/JITStubRoutineSet.h:
+        (JSC::JITStubRoutineSet::deleteUnmarkedJettisonedStubRoutines):
+        (JSC::JITStubRoutineSet::traceMarkedStubRoutines):
+        (JSC::JITStubRoutineSet::mayHaveRoutinesToDelete): Deleted.
+        (JSC::JITStubRoutineSet::notifyHaveRoutinesToDelete): Deleted.
+        * jit/GCAwareJITStubRoutine.cpp:
+        (JSC::GCAwareJITStubRoutine::observeZeroRefCount):
+        * jit/JITStubRoutine.h:
+        (JSC::JITStubRoutine::createSelfManagedRoutine):
+
 2021-07-12  Yijia Huang  <yijia_huang@apple.com>
 

trunk/Source/JavaScriptCore/heap/Heap.cpp

@@ -975 +975 @@
     // And CodeBlock destructor is assuming that CodeBlock gets destroyed before UnlinkedCodeBlock gets destroyed.
     vm().forEachCodeBlockSpace([] (auto& space) { space.space.sweep(); });
-    if (mayHaveJITStubRoutinesToDelete())
-        deleteDeadJITStubRoutines(5_ms);
+    m_jitStubRoutines->deleteUnmarkedJettisonedStubRoutines();
 }
 
@@ -1043 +1042 @@
     m_objectSpace.sweepBlocks();
     m_objectSpace.shrink();
-
-    unsigned passes = 0;
-    while (mayHaveJITStubRoutinesToDelete()) {
-        constexpr Seconds unlimitedTime = 600_s;
-        deleteDeadJITStubRoutines(unlimitedTime);
-        RELEASE_ASSERT(passes++ < 100);
-    }
-
     if (UNLIKELY(Options::logGC())) {
         MonotonicTime after = MonotonicTime::now();

trunk/Source/JavaScriptCore/heap/Heap.h

@@ -407 +407 @@
     bool isMarkingForGCVerifier() const { return m_isMarkingForGCVerifier; }
 
-    static bool mayHaveJITStubRoutinesToDelete();
-    void deleteDeadJITStubRoutines(Seconds timeSlice);
-
 private:
     friend class AllocatingScope;

trunk/Source/JavaScriptCore/heap/HeapInlines.h

@@ -30 +30 @@
 #include "HeapCellInlines.h"
 #include "IndexingHeader.h"
-#include "JITStubRoutineSet.h"
 #include "JSCast.h"
 #include "Structure.h"
@@ -281 +280 @@
 }
 
-inline bool Heap::mayHaveJITStubRoutinesToDelete()
-{
-    return JITStubRoutineSet::mayHaveRoutinesToDelete();
-}
-
-inline void Heap::deleteDeadJITStubRoutines(Seconds timeSlice)
-{
-    m_jitStubRoutines->deleteUnmarkedJettisonedStubRoutines(timeSlice);
-}
-
 } // namespace JSC

trunk/Source/JavaScriptCore/heap/IncrementalSweeper.cpp

@@ -37 +37 @@
 static constexpr double sweepTimeTotal = .10;
 static constexpr double sweepTimeMultiplier = 1.0 / sweepTimeTotal;
-static constexpr Seconds deleteJITStubRoutinesTimeSlice = std::min(sweepTimeSlice / 10, 1_ms);
 
 void IncrementalSweeper::scheduleTimer()
@@ -57 +56 @@
 void IncrementalSweeper::doSweep(VM& vm, MonotonicTime sweepBeginTime)
 {
-    bool hasMoreBlocksToSweep = true;
-    bool hasMoreWork = true;
-    while (hasMoreWork) {
-        if (hasMoreBlocksToSweep)
-            hasMoreBlocksToSweep = sweepNextBlock(vm);
-
-        if (Heap::mayHaveJITStubRoutinesToDelete())
-            vm.heap.deleteDeadJITStubRoutines(deleteJITStubRoutinesTimeSlice);
-
-        hasMoreWork = hasMoreBlocksToSweep || Heap::mayHaveJITStubRoutinesToDelete();
-
+    while (sweepNextBlock(vm)) {
         Seconds elapsedTime = MonotonicTime::now() - sweepBeginTime;
         if (elapsedTime < sweepTimeSlice)

trunk/Source/JavaScriptCore/heap/JITStubRoutineSet.cpp

@@ -30 +30 @@
 
 #include "GCAwareJITStubRoutine.h"
-#include <algorithm>
 
 namespace JSC {
-
-using WTF::Range;
-
-bool JITStubRoutineSet::s_mayHaveRoutinesToDelete = false;
 
 JITStubRoutineSet::JITStubRoutineSet() { }
@@ -120 +115 @@
 }
 
-void JITStubRoutineSet::deleteUnmarkedJettisonedStubRoutines(Seconds timeSlice)
+void JITStubRoutineSet::deleteUnmarkedJettisonedStubRoutines()
 {
-    ASSERT(s_mayHaveRoutinesToDelete);
-
-    MonotonicTime startTime = MonotonicTime::now();
-    Seconds elapsedTime;
-    constexpr unsigned maxBatchSize = 100;
-
-    unsigned endIndex = m_routines.size();
-
-    // Clear the s_mayHaveRoutinesToDelete flag before we start.
-    // Destruction of a MarkingGCAwareJITStubRoutine can trigger more routines
-    // to be deleted, and some of those may be the ones we have already iterated
-    // pass.
-    s_mayHaveRoutinesToDelete = false;
-
     unsigned srcIndex = 0;
-    while (srcIndex < endIndex) {
-        unsigned batchSize = std::min<unsigned>(maxBatchSize, endIndex - srcIndex);
-        while (batchSize--) {
-            Routine routine = m_routines[srcIndex];
-            if (!routine.routine->m_isJettisoned || routine.routine->m_mayBeExecuting) {
-                srcIndex++;
-                continue;
-            }
-            m_routines[srcIndex] = m_routines[--endIndex];
-
-            routine.routine->deleteFromGC();
+    unsigned dstIndex = srcIndex;
+    while (srcIndex < m_routines.size()) {
+        Routine routine = m_routines[srcIndex++];
+        if (!routine.routine->m_isJettisoned || routine.routine->m_mayBeExecuting) {
+            m_routines[dstIndex++] = routine;
+            continue;
         }
-
-        elapsedTime = MonotonicTime::now() - startTime;
-        if (elapsedTime > timeSlice) {
-            // We timed out. Assume there's more to do, and that we should check
-            // again next time slice.
-            s_mayHaveRoutinesToDelete = true;
-            break;
-        }
+        routine.routine->deleteFromGC();
     }
-
-    m_routines.shrinkCapacity(endIndex);
+    m_routines.shrinkCapacity(dstIndex);
 }
 

trunk/Source/JavaScriptCore/heap/JITStubRoutineSet.h

@@ -32 +32 @@
 #include <wtf/Vector.h>
 
+using WTF::Range;
+
 namespace JSC {
 
@@ -60 +62 @@
     void prepareForConservativeScan();
     
-    void deleteUnmarkedJettisonedStubRoutines(Seconds timeSlice);
+    void deleteUnmarkedJettisonedStubRoutines();
 
     template<typename Visitor> void traceMarkedStubRoutines(Visitor&);
-
-    static bool mayHaveRoutinesToDelete() { return s_mayHaveRoutinesToDelete; }
-    static void notifyHaveRoutinesToDelete() { s_mayHaveRoutinesToDelete = true; }
-
+    
 private:
     void markSlow(uintptr_t address);
@@ -75 +74 @@
     };
     Vector<Routine> m_routines;
-    WTF::Range<uintptr_t> m_range { 0, 0 };
-
-    static bool s_mayHaveRoutinesToDelete;
+    Range<uintptr_t> m_range { 0, 0 };
 };
 
@@ -94 +91 @@
     void mark(void*) { }
     void prepareForConservativeScan() { }
-    void deleteUnmarkedJettisonedStubRoutines(Seconds) { }
+    void deleteUnmarkedJettisonedStubRoutines() { }
     template<typename Visitor> void traceMarkedStubRoutines(Visitor&) { }
-
-    static bool mayHaveRoutinesToDelete() { return false; }
-    static void notifyHaveRoutinesToDelete() { }
 };
 

trunk/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp

@@ -68 +68 @@
 
     m_isJettisoned = true;
-    JITStubRoutineSet::notifyHaveRoutinesToDelete();
 }
 

trunk/Source/JavaScriptCore/jit/JITStubRoutine.h

@@ -1 +1 @@
 /*
- * Copyright (C) 2012-2021 Apple Inc. All rights reserved.
+ * Copyright (C) 2012-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -64 +64 @@
     }
     
+    // Use this if you want to pass a CodePtr to someone who insists on taking
+    // a RefPtr<JITStubRoutine>.
+    static Ref<JITStubRoutine> createSelfManagedRoutine(
+        MacroAssemblerCodePtr<JITStubRoutinePtrTag> rawCodePointer)
+    {
+        return adoptRef(*new JITStubRoutine(MacroAssemblerCodeRef<JITStubRoutinePtrTag>::createSelfManagedCodeRef(rawCodePointer)));
+    }
+    
     virtual ~JITStubRoutine();
     virtual void aboutToDie() { }