xo-gc: refactor for MutationLogStore bugs [TESTFAIL]

include/xo/gc/MutationLogEntry.hpp

@@ -9,6 +9,7 @@
 
 namespace xo {
     namespace mm {
+        class GCObjectStore;  // see xo-gc/ GCObjectStore.hpp
 
         /** @brief Track a cross-generational pointer
          *
@@ -40,6 +41,15 @@ namespace xo {
             /** true iff child pointer has been altered since this mlog entry created **/
             bool is_superseded() const noexcept { return *p_data_ != snap_.data(); }
 
+            /** Refresh snapshot when *p_data_ does not match snap_.data_
+             *  Get updated facet information from destination alloc header.
+             *  It's possible that *p_data_ no longer points to gc-owned space
+             *
+             *  @return true if snapshot updated. false if this entry should be discarded
+             **/
+            bool refresh_snapshot(Generation parent_gen,
+                                  GCObjectStore * gcos) noexcept;
+
         private:
             /** address of object containing logged mutation **/
             void * parent_ = nullptr;

include/xo/gc/MutationLogStore.hpp

@@ -139,18 +139,19 @@ namespace xo {
             MutationLogStatistics _preserve_child_of_live_parent(obj<AGCObjectVisitor> gc,
                                                                  Generation upto,
                                                                  Generation parent_gen,
-                                                                 const MutationLogEntry & from_entry,
+                                                                 MutationLogEntry & from_entry,
                                                                  MutationLog * keep_mlog);
 
+#ifdef OBSOLETE
             /** On behalf of collector @p gc:
              *
              *  helper function to decide whether to keep a mutation log entry
-             *  @return true iff mlog entry appended to @p keep_mlog
              **/
-            bool _check_keep_mutation_aux(const MutationLogEntry & from_entry,
+            void _check_keep_mutation_aux(MutationLogEntry & from_entry,
                                           Generation parent_gen_to,
                                           void * child_to,
                                           MutationLog * keep_mlog);
+#endif
 
 
         public:

src/gc/MutationLogEntry.cpp

@@ -4,8 +4,11 @@
  **/
 
 #include "MutationLogEntry.hpp"
+#include <xo/gc/GCObjectStore.hpp>
 
 namespace xo {
+    using xo::reflect::typeseq;
+
     namespace mm {
 
         MutationLogEntry::MutationLogEntry(void * parent,
@@ -16,6 +19,71 @@ namespace xo {
                   snap_{snap}
         {}
 
+        bool
+        MutationLogEntry::refresh_snapshot(Generation parent_gen,
+                                           GCObjectStore * gcos) noexcept
+        {
+            void * child_data = *p_data_;
+            // note: never the same child_info as computed at the top of
+            //       MutationLogEntry._preserve_child_of_live_parent()
+            //       Child pointer was either forwarding pointer or moved.
+            //       In either case must pickup info for new location.
+            //
+            AllocInfo child_info = gcos->alloc_info((std::byte*)child_data);
+
+            if (child_info.is_forwarding_tseq()) {
+                // code salvaged from MutationLogStore._check_keep_mutation_aux()
+                // as reminder.  But if caller is gc will have to know this anyway,
+                // so it can move child
+
+                // if (info.is_forwarding_tseq()) {
+                //    child_data = *(void **)child_data;
+                //    info = gcos->alloc_info((std::byte *)child_data);
+                //}
+
+                assert(false);  // for now assuming caller forward child
+            }
+
+            Generation child_gen_to
+                = gcos->generation_of(Role::to_space(), child_data);
+
+            if (child_gen_to.is_sentinel()) {
+                // child no longer points to gc-owned space.
+                // 1. may not have an alloc header (could be a static global for example),
+                //    so AllocInfo not available
+                // 2. doesn't need a mutation log entry since this gc can't move destination
+
+                snap_.data_ = nullptr; // hygiene
+
+                return false;
+            }
+
+            const GCObjectStoreConfig & config = gcos->config();
+
+            bool need_mlog_entry
+                = ((child_gen_to + 1 < config.n_generation_)
+                   && (config.promotion_threshold(parent_gen)
+                       > config.promotion_threshold(child_gen_to)));
+
+            if (need_mlog_entry) {
+                AGCObject * iface = gcos->lookup_type(typeseq(child_info.tseq()));
+
+                if (iface) {
+                    this->snap_ = obj<AGCObject>(iface, child_data);
+
+                    // snapshot updated, keep mlog entry
+                    return true;
+                } else {
+                    assert(false);
+
+                    return false;
+                }
+            } else {
+                // retire this entry.
+                return false;
+            }
+        }
+
     } /*namespace mm*/
 } /*namespace xo*/
 

src/gc/MutationLogStore.cpp

@@ -412,7 +412,8 @@ namespace xo {
                         assert(!parent_gen_to.is_sentinel());
 
                         // Since parent already forwarded, we don't have to preserve child
-                        // or update parent object.
+                        // or update parent object. GC already guaranteed to have visited
+                        // parent's child pointers
                         //
                         // Do need to replace mlog entry to reflect new parent location.
 
@@ -421,14 +422,19 @@ namespace xo {
                                - (std::byte *)from_entry.parent());
 
                         void ** p_data_to = (void **)((std::byte *)(parent_to) + offset);
-                        void * child_to = *p_data_to;
+                        //void * child_to = *p_data_to;
 
                         MutationLogEntry to_entry(parent_to, p_data_to, from_entry.snap());
 
+                        if (to_entry.refresh_snapshot(parent_gen_to, gco_store_))
+                            keep_mlog->push_back(to_entry);
+
+#ifdef OBSOLETE
                         this->_check_keep_mutation_aux(to_entry,
                                                        parent_gen_to,
                                                        child_to,
                                                        keep_mlog);
+#endif
 
 
                     } else {
@@ -459,7 +465,7 @@ namespace xo {
         MutationLogStore::_preserve_child_of_live_parent(obj<AGCObjectVisitor> gc,
                                                          Generation upto,
                                                          Generation parent_gen,
-                                                         const MutationLogEntry & from_entry,
+                                                         MutationLogEntry & from_entry,
                                                          MutationLog * keep_mlog)
         {
             void * child_fr = *from_entry.p_data();
@@ -487,29 +493,31 @@ namespace xo {
                 // TODO: make this a method on AllocInfo
                 child_to  = *(void **)child_fr;
 
-                // assigning through address of P->C pointer
-                // also makes mlog entry current
-
             } else {
                 // [MLOG2]
 
                 ++counters.n_rescue_;
 
                 child_to = gco_store_->deep_move_interior(gc, child_fr, upto);
+            }
 
-                // update child pointer in parent object
+            // update child pointer in parent object.
-                *from_entry.p_data() = child_to;
+            // either forwarded or moved
+            *from_entry.p_data() = child_to;
+
+            // TODO: pass statistics object
+            if (from_entry.refresh_snapshot(parent_gen, gco_store_)) {
+                keep_mlog->push_back(from_entry);
             }
 
             // child_to generation in {gen, gen+1}
 
-            this->_check_keep_mutation_aux(from_entry, parent_gen, child_to, keep_mlog);
-
             return counters;
         }
 
-        bool
+#ifdef OBSOLETE
-        MutationLogStore::_check_keep_mutation_aux(const MutationLogEntry & from_entry,
+        void
+        MutationLogStore::_check_keep_mutation_aux(MutationLogEntry & from_entry,
                                                    Generation parent_gen_to,
                                                    void * child_to,
                                                    MutationLog * keep_mlog)
@@ -539,6 +547,7 @@ namespace xo {
                 return false;
             }
         }
+#endif
 
 
     } /*namespace mm*/

utest/MutationLogStore.test.cpp

@@ -212,6 +212,51 @@ namespace ut {
 
         // ----------------------------------------------------------------
 
+        static Step step_4[] = {
+            // ----- phase 0 -----
+            {Cmd::make_bool,   0, 0}, // [0]: #f
+            {Cmd::make_bool,   1, 0}, // [1]: #t
+            {Cmd::make_nil,    0, 0}, // [2]: #nil
+            {Cmd::make_cons,   0, 2}, // [3]: cons(#f,#nil)
+
+            // 1st gc
+
+            // ----- phase 1 -----
+            {Cmd::make_bool,   1, 0}, // [4]: #t
+            {Cmd::assign_head, 3, 4}, //      set-car([3],#t)
+
+            // 2nd gc. [0]..[3] promote to g1
+            // [4] in g0 so [3]->[4] requires mlog entry
+
+            // ----- phase 2 -----
+            {Cmd::make_bool,   0, 0}, // [5]: #f
+            {Cmd::assign_head, 3, 5}, //      set-car([3],#f)
+
+            // 3rd gc. [4] promotes to g1,
+            // [5] in g0 so [3]->[5] requires mlog entry
+
+            // ----- phase 3 -----
+            // ----- phase 4 -----
+            // ----- end -----
+            {Cmd::sentinel,    0, 0},
+        };
+
+        static Phase phase_4[] = {
+            //
+            // lo   hi    mlog_new_z_[]
+            //  v    v    v
+            {   0,   4,   {0} },  // phase 0 gc
+            {   4,   6,   {1} },  // phase 1 gc. set-car makes 1x xage ptr
+            {   6,   8,   {2} },  // phase 2 gc. now src in g1, dest [4] in g0
+            {   8,   8,   {1} },  // phase 3 gc. new dest [5] in g0
+            {   8,   8,   {0} },  // phase 4 gc. now dest in g1
+            {  -1,  -1,   {0} },
+        };
+
+        static TestSequence seq_4 { step_4, phase_4 };
+
+        // ----------------------------------------------------------------
+
 #      define seq_nil TestSequence{}
 #      define nil nullptr
 #      define T true
@@ -239,7 +284,8 @@ namespace ut {
             Testcase(2, 4, 16 * 1024, 8 * 128, T,   seq_0,    0, F,     c_fixed, 1,  0,  0,  0,  0, F),
             Testcase(2, 4, 16 * 1024, 8 * 128, T,   seq_1,    0, F,     c_fixed, 1,  0,  0,  0,  0, F),
             Testcase(2, 1, 16 * 1024, 8 * 128, T,   seq_2,  128, T,     c_fixed, 3,  0,  0,  0,  0, F),
-            Testcase(2, 2, 16 * 1024, 8 * 128, T,   seq_3,  128, T,     c_fixed, 4,  0,  0,  0,  0, T),
+            Testcase(2, 2, 16 * 1024, 8 * 128, T,   seq_3,  128, T,     c_fixed, 4,  0,  0,  0,  0, F),
+            Testcase(2, 2, 16 * 1024, 8 * 128, T,   seq_4,  128, T,     c_fixed, 4,  0,  0,  0,  0, T),
         };
 
 #      undef T
@@ -376,6 +422,8 @@ namespace ut {
             for (uint32_t loop_index = 0; loop_index < tc.n_gc_loop_; ++loop_index) {
                 scope log2(XO_DEBUG(tc.debug_flag_), "gc loop", xtag("loop_index", loop_index));
 
+                INFO(xtag("loop_index", loop_index));
+
                 GcosTestutil::gcos_construct_ab_object_graphs(tc.test_seq_,
                                                               tc.obj_graph_type_,
                                                               tc.n_i0_test_obj_,
@@ -442,9 +490,8 @@ namespace ut {
 
                     REQUIRE(gcos.verify_stats()->is_ok());
                 }
-            }
+            } /*one gc cycle per loop*/
-        }
+        } /*testcase loop*/
-
     }
 } /*namespace ut*/