Title: [279861] trunk/Source/_javascript_Core
Revision
279861
Author
mark....@apple.com
Date
2021-07-12 18:47:47 -0700 (Mon, 12 Jul 2021)

Log 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):

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/CMakeLists.txt (279860 => 279861)


--- trunk/Source/_javascript_Core/CMakeLists.txt	2021-07-13 01:22:14 UTC (rev 279860)
+++ trunk/Source/_javascript_Core/CMakeLists.txt	2021-07-13 01:47:47 UTC (rev 279861)
@@ -670,7 +670,6 @@
     heap/IsoSubspace.h
     heap/IsoSubspaceInlines.h
     heap/IsoSubspacePerVM.h
-    heap/JITStubRoutineSet.h
     heap/LocalAllocator.h
     heap/LocalAllocatorInlines.h
     heap/MachineStackMarker.h

Modified: trunk/Source/_javascript_Core/ChangeLog (279860 => 279861)


--- trunk/Source/_javascript_Core/ChangeLog	2021-07-13 01:22:14 UTC (rev 279860)
+++ trunk/Source/_javascript_Core/ChangeLog	2021-07-13 01:47:47 UTC (rev 279861)
@@ -1,3 +1,39 @@
+2021-07-12  Mark Lam  <mark....@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_hu...@apple.com>
 
         Add SMNEGL, UMNEGL, UMADDL, and UMSUBL for ARM64 and select this instruction in Air

Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (279860 => 279861)


--- trunk/Source/_javascript_Core/heap/Heap.cpp	2021-07-13 01:22:14 UTC (rev 279860)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp	2021-07-13 01:47:47 UTC (rev 279861)
@@ -974,8 +974,7 @@
     // Sweeping must occur before deleting stubs, otherwise the stubs might still think they're alive as they get deleted.
     // 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();
 }
 
 void Heap::addToRememberedSet(const JSCell* constCell)
@@ -1042,14 +1041,6 @@
     }
     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();
         dataLog("=> ", capacity() / 1024, "kb, ", (after - before).milliseconds(), "ms");

Modified: trunk/Source/_javascript_Core/heap/Heap.h (279860 => 279861)


--- trunk/Source/_javascript_Core/heap/Heap.h	2021-07-13 01:22:14 UTC (rev 279860)
+++ trunk/Source/_javascript_Core/heap/Heap.h	2021-07-13 01:47:47 UTC (rev 279861)
@@ -406,9 +406,6 @@
 
     bool isMarkingForGCVerifier() const { return m_isMarkingForGCVerifier; }
 
-    static bool mayHaveJITStubRoutinesToDelete();
-    void deleteDeadJITStubRoutines(Seconds timeSlice);
-
 private:
     friend class AllocatingScope;
     friend class CodeBlock;

Modified: trunk/Source/_javascript_Core/heap/HeapInlines.h (279860 => 279861)


--- trunk/Source/_javascript_Core/heap/HeapInlines.h	2021-07-13 01:22:14 UTC (rev 279860)
+++ trunk/Source/_javascript_Core/heap/HeapInlines.h	2021-07-13 01:47:47 UTC (rev 279861)
@@ -29,7 +29,6 @@
 #include "Heap.h"
 #include "HeapCellInlines.h"
 #include "IndexingHeader.h"
-#include "JITStubRoutineSet.h"
 #include "JSCast.h"
 #include "Structure.h"
 #include <type_traits>
@@ -280,14 +279,4 @@
         func(*visitor);
 }
 
-inline bool Heap::mayHaveJITStubRoutinesToDelete()
-{
-    return JITStubRoutineSet::mayHaveRoutinesToDelete();
-}
-
-inline void Heap::deleteDeadJITStubRoutines(Seconds timeSlice)
-{
-    m_jitStubRoutines->deleteUnmarkedJettisonedStubRoutines(timeSlice);
-}
-
 } // namespace JSC

Modified: trunk/Source/_javascript_Core/heap/IncrementalSweeper.cpp (279860 => 279861)


--- trunk/Source/_javascript_Core/heap/IncrementalSweeper.cpp	2021-07-13 01:22:14 UTC (rev 279860)
+++ trunk/Source/_javascript_Core/heap/IncrementalSweeper.cpp	2021-07-13 01:47:47 UTC (rev 279861)
@@ -36,7 +36,6 @@
 static constexpr Seconds sweepTimeSlice = 10_ms;
 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()
 {
@@ -56,17 +55,7 @@
 
 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)
             continue;

Modified: trunk/Source/_javascript_Core/heap/JITStubRoutineSet.cpp (279860 => 279861)


--- trunk/Source/_javascript_Core/heap/JITStubRoutineSet.cpp	2021-07-13 01:22:14 UTC (rev 279860)
+++ trunk/Source/_javascript_Core/heap/JITStubRoutineSet.cpp	2021-07-13 01:47:47 UTC (rev 279861)
@@ -29,14 +29,9 @@
 #if ENABLE(JIT)
 
 #include "GCAwareJITStubRoutine.h"
-#include <algorithm>
 
 namespace JSC {
 
-using WTF::Range;
-
-bool JITStubRoutineSet::s_mayHaveRoutinesToDelete = false;
-
 JITStubRoutineSet::JITStubRoutineSet() { }
 JITStubRoutineSet::~JITStubRoutineSet()
 {
@@ -119,46 +114,19 @@
     }
 }
 
-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);
 }
 
 template<typename Visitor>

Modified: trunk/Source/_javascript_Core/heap/JITStubRoutineSet.h (279860 => 279861)


--- trunk/Source/_javascript_Core/heap/JITStubRoutineSet.h	2021-07-13 01:22:14 UTC (rev 279860)
+++ trunk/Source/_javascript_Core/heap/JITStubRoutineSet.h	2021-07-13 01:47:47 UTC (rev 279861)
@@ -31,6 +31,8 @@
 #include <wtf/Range.h>
 #include <wtf/Vector.h>
 
+using WTF::Range;
+
 namespace JSC {
 
 class GCAwareJITStubRoutine;
@@ -59,13 +61,10 @@
 
     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);
     
@@ -74,9 +73,7 @@
         GCAwareJITStubRoutine* routine;
     };
     Vector<Routine> m_routines;
-    WTF::Range<uintptr_t> m_range { 0, 0 };
-
-    static bool s_mayHaveRoutinesToDelete;
+    Range<uintptr_t> m_range { 0, 0 };
 };
 
 #else // !ENABLE(JIT)
@@ -93,11 +90,8 @@
     void clearMarks() { }
     void mark(void*) { }
     void prepareForConservativeScan() { }
-    void deleteUnmarkedJettisonedStubRoutines(Seconds) { }
+    void deleteUnmarkedJettisonedStubRoutines() { }
     template<typename Visitor> void traceMarkedStubRoutines(Visitor&) { }
-
-    static bool mayHaveRoutinesToDelete() { return false; }
-    static void notifyHaveRoutinesToDelete() { }
 };
 
 #endif // !ENABLE(JIT)

Modified: trunk/Source/_javascript_Core/jit/GCAwareJITStubRoutine.cpp (279860 => 279861)


--- trunk/Source/_javascript_Core/jit/GCAwareJITStubRoutine.cpp	2021-07-13 01:22:14 UTC (rev 279860)
+++ trunk/Source/_javascript_Core/jit/GCAwareJITStubRoutine.cpp	2021-07-13 01:47:47 UTC (rev 279861)
@@ -67,7 +67,6 @@
     RELEASE_ASSERT(!m_refCount);
 
     m_isJettisoned = true;
-    JITStubRoutineSet::notifyHaveRoutinesToDelete();
 }
 
 void GCAwareJITStubRoutine::deleteFromGC()

Modified: trunk/Source/_javascript_Core/jit/JITStubRoutine.h (279860 => 279861)


--- trunk/Source/_javascript_Core/jit/JITStubRoutine.h	2021-07-13 01:22:14 UTC (rev 279860)
+++ trunk/Source/_javascript_Core/jit/JITStubRoutine.h	2021-07-13 01:47:47 UTC (rev 279861)
@@ -1,5 +1,5 @@
 /*
- * 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
  * modification, are permitted provided that the following conditions
@@ -63,6 +63,14 @@
     {
     }
     
+    // 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() { }
     
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to