Merge pull request #646 from kmod/class_freeing

Temporary fix for freeing-classes-before-instances bug

src/gc/collector.cpp

@@ -375,10 +375,10 @@ void markPhase() {
 #endif
 }
 
-static void sweepPhase(std::vector<Box*>& weakly_referenced) {
+static void sweepPhase(std::vector<Box*>& weakly_referenced, std::vector<BoxedClass*>& classes_to_free) {
     // we need to use the allocator here because these objects are referenced only here, and calling the weakref
     // callbacks could start another gc
-    global_heap.freeUnmarked(weakly_referenced);
+    global_heap.freeUnmarked(weakly_referenced, classes_to_free);
 }
 
 static bool gc_enabled = true;
@@ -433,7 +433,17 @@ void runCollection() {
     // since the deallocation of other objects (namely, the weakref objects themselves) can affect
     // those lists, and we want to see the final versions.
     std::vector<Box*> weakly_referenced;
-    sweepPhase(weakly_referenced);
+
+    // Temporary solution to the "we can't free classes before their instances": the sweep phase
+    // will avoid freeing classes, and will instead put them into this list for us to free at the end.
+    // XXX there are still corner cases with it:
+    // - there is no ordering enforced between different class objects, ie if you free a class and a metaclass
+    //   in the same collection we have the same issue
+    // - if there are weakreferences to the class, it gets freed slightly earlier
+    // These could both be fixed but I think the full fix will come with rudi's larger finalization changes.
+    std::vector<BoxedClass*> classes_to_free;
+
+    sweepPhase(weakly_referenced, classes_to_free);
 
     // Handle weakrefs in two passes:
     // - first, find all of the weakref objects whose callbacks we need to call.  we need to iterate
@@ -458,7 +468,11 @@ void runCollection() {
                     weak_references.push_back(head);
             }
         }
-        global_heap.free(GCAllocation::fromUserData(o));
+        global_heap._setFree(GCAllocation::fromUserData(o));
+    }
+
+    for (auto b : classes_to_free) {
+        global_heap._setFree(GCAllocation::fromUserData(b));
     }
 
     should_not_reenter_gc = false; // end non-reentrant section

src/gc/heap.cpp

@@ -43,7 +43,7 @@ template <> void return_temporary_buffer<pyston::Box*>(pyston::Box** p) {
 namespace pyston {
 namespace gc {
 
-bool _doFree(GCAllocation* al, std::vector<Box*>* weakly_referenced);
+bool _doFree(GCAllocation* al, std::vector<Box*>* weakly_referenced, std::vector<BoxedClass*>* classes_to_free);
 
 // lots of linked lists around here, so let's just use template functions for operations on them.
 template <class ListT> inline void nullNextPrev(ListT* node) {
@@ -86,7 +86,8 @@ template <class ListT, typename Func> inline void forEach(ListT* list, Func func
 }
 
 template <class ListT, typename Free>
-inline void sweepList(ListT* head, std::vector<Box*>& weakly_referenced, Free free_func) {
+inline void sweepList(ListT* head, std::vector<Box*>& weakly_referenced, std::vector<BoxedClass*>& classes_to_free,
+                      Free free_func) {
     auto cur = head;
     while (cur) {
         GCAllocation* al = cur->data;
@@ -94,7 +95,7 @@ inline void sweepList(ListT* head, std::vector<Box*>& weakly_referenced, Free fr
             clearMark(al);
             cur = cur->next;
         } else {
-            if (_doFree(al, &weakly_referenced)) {
+            if (_doFree(al, &weakly_referenced, &classes_to_free)) {
                 removeFromLL(cur);
 
                 auto to_free = cur;
@@ -122,7 +123,8 @@ void _bytesAllocatedTripped() {
 
 Heap global_heap;
 
-__attribute__((always_inline)) bool _doFree(GCAllocation* al, std::vector<Box*>* weakly_referenced) {
+__attribute__((always_inline)) bool _doFree(GCAllocation* al, std::vector<Box*>* weakly_referenced,
+                                            std::vector<BoxedClass*>* classes_to_free) {
 #ifndef NVALGRIND
     VALGRIND_DISABLE_ERROR_REPORTING;
 #endif
@@ -149,6 +151,13 @@ __attribute__((always_inline)) bool _doFree(GCAllocation* al, std::vector<Box*>*
             }
         }
 
+        // Note: do this check after the weakrefs check.
+        if (PyType_Check(b)) {
+            assert(classes_to_free);
+            classes_to_free->push_back(static_cast<BoxedClass*>(b));
+            return false;
+        }
+
         // XXX: we are currently ignoring destructors (tp_dealloc) for extension objects, since we have
         // historically done that (whoops) and there are too many to be worth changing for now as long
         // as we can get real destructor support soon.
@@ -161,7 +170,7 @@ __attribute__((always_inline)) bool _doFree(GCAllocation* al, std::vector<Box*>*
 }
 
 void Heap::destructContents(GCAllocation* al) {
-    _doFree(al, NULL);
+    _doFree(al, NULL, NULL);
 }
 
 struct HeapStatistics {
@@ -369,8 +378,8 @@ GCAllocation* SmallArena::allocationFrom(void* ptr) {
     return reinterpret_cast<GCAllocation*>(&b->atoms[atom_idx]);
 }
 
-void SmallArena::freeUnmarked(std::vector<Box*>& weakly_referenced) {
-    thread_caches.forEachValue([this, &weakly_referenced](ThreadBlockCache* cache) {
+void SmallArena::freeUnmarked(std::vector<Box*>& weakly_referenced, std::vector<BoxedClass*>& classes_to_free) {
+    thread_caches.forEachValue([this, &weakly_referenced, &classes_to_free](ThreadBlockCache* cache) {
         for (int bidx = 0; bidx < NUM_BUCKETS; bidx++) {
             Block* h = cache->cache_free_heads[bidx];
             // Try to limit the amount of unused memory a thread can hold onto;
@@ -390,8 +399,8 @@ void SmallArena::freeUnmarked(std::vector<Box*>& weakly_referenced) {
                 insertIntoLL(&heads[bidx], h);
             }
 
-            Block** chain_end = _freeChain(&cache->cache_free_heads[bidx], weakly_referenced);
-            _freeChain(&cache->cache_full_heads[bidx], weakly_referenced);
+            Block** chain_end = _freeChain(&cache->cache_free_heads[bidx], weakly_referenced, classes_to_free);
+            _freeChain(&cache->cache_full_heads[bidx], weakly_referenced, classes_to_free);
 
             while (Block* b = cache->cache_full_heads[bidx]) {
                 removeFromLLAndNull(b);
@@ -401,8 +410,8 @@ void SmallArena::freeUnmarked(std::vector<Box*>& weakly_referenced) {
     });
 
     for (int bidx = 0; bidx < NUM_BUCKETS; bidx++) {
-        Block** chain_end = _freeChain(&heads[bidx], weakly_referenced);
-        _freeChain(&full_heads[bidx], weakly_referenced);
+        Block** chain_end = _freeChain(&heads[bidx], weakly_referenced, classes_to_free);
+        _freeChain(&full_heads[bidx], weakly_referenced, classes_to_free);
 
         while (Block* b = full_heads[bidx]) {
             removeFromLLAndNull(b);
@@ -429,7 +438,8 @@ void SmallArena::getStatistics(HeapStatistics* stats) {
 }
 
 
-SmallArena::Block** SmallArena::_freeChain(Block** head, std::vector<Box*>& weakly_referenced) {
+SmallArena::Block** SmallArena::_freeChain(Block** head, std::vector<Box*>& weakly_referenced,
+                                           std::vector<BoxedClass*>& classes_to_free) {
     while (Block* b = *head) {
         int num_objects = b->numObjects();
         int first_obj = b->minObjIndex();
@@ -453,7 +463,7 @@ SmallArena::Block** SmallArena::_freeChain(Block** head, std::vector<Box*>& weak
             if (isMarked(al)) {
                 clearMark(al);
             } else {
-                if (_doFree(al, &weakly_referenced)) {
+                if (_doFree(al, &weakly_referenced, &classes_to_free)) {
                     b->isfree.set(atom_idx);
 #ifndef NDEBUG
                     memset(al->user_data, 0xbb, b->size - sizeof(GCAllocation));
@@ -664,8 +674,8 @@ GCAllocation* LargeArena::allocationFrom(void* ptr) {
     return NULL;
 }
 
-void LargeArena::freeUnmarked(std::vector<Box*>& weakly_referenced) {
-    sweepList(head, weakly_referenced, [this](LargeObj* ptr) { _freeLargeObj(ptr); });
+void LargeArena::freeUnmarked(std::vector<Box*>& weakly_referenced, std::vector<BoxedClass*>& classes_to_free) {
+    sweepList(head, weakly_referenced, classes_to_free, [this](LargeObj* ptr) { _freeLargeObj(ptr); });
 }
 
 void LargeArena::getStatistics(HeapStatistics* stats) {
@@ -857,8 +867,8 @@ GCAllocation* HugeArena::allocationFrom(void* ptr) {
     return NULL;
 }
 
-void HugeArena::freeUnmarked(std::vector<Box*>& weakly_referenced) {
-    sweepList(head, weakly_referenced, [this](HugeObj* ptr) { _freeHugeObj(ptr); });
+void HugeArena::freeUnmarked(std::vector<Box*>& weakly_referenced, std::vector<BoxedClass*>& classes_to_free) {
+    sweepList(head, weakly_referenced, classes_to_free, [this](HugeObj* ptr) { _freeHugeObj(ptr); });
 }
 
 void HugeArena::getStatistics(HeapStatistics* stats) {

src/gc/heap.h

@@ -223,7 +223,7 @@ public:
     void free(GCAllocation* al);
 
     GCAllocation* allocationFrom(void* ptr);
-    void freeUnmarked(std::vector<Box*>& weakly_referenced);
+    void freeUnmarked(std::vector<Box*>& weakly_referenced, std::vector<BoxedClass*>& classes_to_free);
 
     void getStatistics(HeapStatistics* stats);
 
@@ -355,7 +355,7 @@ private:
     Block* _allocBlock(uint64_t size, Block** prev);
     GCAllocation* _allocFromBlock(Block* b);
     Block* _claimBlock(size_t rounded_size, Block** free_head);
-    Block** _freeChain(Block** head, std::vector<Box*>& weakly_referenced);
+    Block** _freeChain(Block** head, std::vector<Box*>& weakly_referenced, std::vector<BoxedClass*>& classes_to_free);
     void _getChainStatistics(HeapStatistics* stats, Block** head);
 
     GCAllocation* __attribute__((__malloc__)) _alloc(size_t bytes, int bucket_idx);
@@ -428,7 +428,7 @@ public:
     void free(GCAllocation* alloc);
 
     GCAllocation* allocationFrom(void* ptr);
-    void freeUnmarked(std::vector<Box*>& weakly_referenced);
+    void freeUnmarked(std::vector<Box*>& weakly_referenced, std::vector<BoxedClass*>& classes_to_free);
 
     void getStatistics(HeapStatistics* stats);
 };
@@ -446,7 +446,7 @@ public:
     void free(GCAllocation* alloc);
 
     GCAllocation* allocationFrom(void* ptr);
-    void freeUnmarked(std::vector<Box*>& weakly_referenced);
+    void freeUnmarked(std::vector<Box*>& weakly_referenced, std::vector<BoxedClass*>& classes_to_free);
 
     void getStatistics(HeapStatistics* stats);
 
@@ -529,18 +529,7 @@ public:
     void free(GCAllocation* alloc) {
         destructContents(alloc);
 
-        if (large_arena.contains(alloc)) {
-            large_arena.free(alloc);
-            return;
-        }
-
-        if (huge_arena.contains(alloc)) {
-            huge_arena.free(alloc);
-            return;
-        }
-
-        assert(small_arena.contains(alloc));
-        small_arena.free(alloc);
+        _setFree(alloc);
     }
 
     // not thread safe:
@@ -557,15 +546,34 @@ public:
     }
 
     // not thread safe:
-    void freeUnmarked(std::vector<Box*>& weakly_referenced) {
-        small_arena.freeUnmarked(weakly_referenced);
-        large_arena.freeUnmarked(weakly_referenced);
-        huge_arena.freeUnmarked(weakly_referenced);
+    void freeUnmarked(std::vector<Box*>& weakly_referenced, std::vector<BoxedClass*>& classes_to_free) {
+        small_arena.freeUnmarked(weakly_referenced, classes_to_free);
+        large_arena.freeUnmarked(weakly_referenced, classes_to_free);
+        huge_arena.freeUnmarked(weakly_referenced, classes_to_free);
     }
 
     void dumpHeapStatistics(int level);
 
+private:
+    // Internal function that just marks the allocation as being freed, without doing any
+    // Python-semantics on it.
+    void _setFree(GCAllocation* alloc) {
+        if (large_arena.contains(alloc)) {
+            large_arena.free(alloc);
+            return;
+        }
+
+        if (huge_arena.contains(alloc)) {
+            huge_arena.free(alloc);
+            return;
+        }
+
+        assert(small_arena.contains(alloc));
+        small_arena.free(alloc);
+    }
+
     friend void markPhase();
+    friend void runCollection();
 };
 
 extern Heap global_heap;

test/tests/class_freeing_with_instances.py

+# Since gc-related metadata is (currently) placed on classes,
+# it's tricky for us if a class object becomes garbage in the same
+# collection as some of its instances.  If we try to collect the class
+# first, we will not have the metadata any more on how to collect the instances.
+
+def f():
+    class C(object):
+        pass
+    for i in xrange(1000):
+        pass
+
+for j in xrange(1000):
+    f()
+
+import gc
+gc.collect()