[Zope3-checkins] CVS: ZODB3/ZODB - cPickleCache.c:1.85.6.4

[Tim Peters]

Update of /cvs-repository/ZODB3/ZODB
In directory cvs.zope.org:/tmp/cvs-serv6721/ZODB

Modified Files:
      Tag: Zope-2_7-branch
	cPickleCache.c 
Log Message:
scan_gc_items():  so-far semantically neutral code and comment cleanups.
Gave it a single exit point because it will soon need one <wink>.  Added
simple insert_after() and unlink_from_ring() utility functions (which
will also simplify what's coming next).


=== ZODB3/ZODB/cPickleCache.c 1.85.6.3 => 1.85.6.4 ===
--- ZODB3/ZODB/cPickleCache.c:1.85.6.3	Tue May 18 11:54:44 2004
+++ ZODB3/ZODB/cPickleCache.c	Tue May 18 14:31:24 2004
@@ -146,6 +146,27 @@
 
 } ccobject;
 
+/* Insert self into the ring, following after. */
+static void
+insert_after(CPersistentRing *self, CPersistentRing *after)
+{
+    assert(self != NULL);
+    assert(after != NULL);
+    self->prev = after;
+    self->next = after->next;
+    after->next->prev = self;
+    after->next = self;
+}
+
+/* Remove self from the ring. */
+static void
+unlink_from_ring(CPersistentRing *self)
+{
+    assert(self != NULL);
+    self->prev->next = self->next;
+    self->next->prev = self->prev;
+}
+
 static int cc_ass_sub(ccobject *self, PyObject *key, PyObject *v);
 
 /* ---------------------------------------------------------------- */
@@ -159,20 +180,18 @@
     /* This function must only be called with the ring lock held,
        because it places a non-object placeholder in the ring.
     */
-
     cPersistentObject *object;
     int error;
     CPersistentRing placeholder;
     CPersistentRing *here = self->ring_home.next;
+    int result = -1;   /* guilty until proved innocent */
 
     /* Scan through the ring until we either find the ring_home (i.e. start
-     * of the ring, or we've ghosted enough objects to reach the target
+     * of the ring), or we've ghosted enough objects to reach the target
      * size.
      */
-    while (1) {
-	/* back to the home position. stop looking */
-        if (here == &self->ring_home)
-            return 0;
+    while (here != &self->ring_home && self->non_ghost_count > target) {
+	assert(self->ring_lock);
 
         /* At this point we know that the ring only contains nodes
 	   from persistent objects, plus our own home node. We know
@@ -180,51 +199,40 @@
 	   the current ring node is a persistent object now we know it
 	   is not the home */
         object = OBJECT_FROM_RING(self, here, "scan_gc_items");
-        if (!object)
-	    return -1;
 
-	/* we are small enough */
-        if (self->non_ghost_count <= target)
-            return 0;
-        else if (object->state == cPersistent_UPTODATE_STATE) {
+        if (object->state == cPersistent_UPTODATE_STATE) {
             /* deactivate it. This is the main memory saver. */
 
-            /* Add a placeholder; a dummy node in the ring.  We need
+            /* Add a placeholder;  a dummy node in the ring.  We need
 	       to do this to mark our position in the ring.  It is
 	       possible that the PyObject_SetAttr() call below will
-	       invoke an __setattr__() hook in Python.  If it does,
-	       another thread might run; if that thread accesses a
-	       persistent object and moves it to the head of the ring,
-	       it might cause the gc scan to start working from the
-	       head of the list.
+	       invoke a __setattr__() hook in Python.  Also possible
+	       that deactivation will lead to a __del__ method call.
+	       So another thread might run, and mutate the ring as a side
+	       effect of object accesses.  There's no predicting then where
+	       in the ring here->next will point after that.  The
+	       placeholder won't move as a side effect of calling Python
+	       code.
 	    */
-
-            placeholder.next = here->next;
-            placeholder.prev = here;
-            here->next->prev = &placeholder;
-            here->next = &placeholder;
-
+            insert_after(&placeholder, here);
             ENGINE_NOISE("G");
 
-            /* In Python, "obj._p_changed = None" spells, ghostify */
+            /* In Python, "obj._p_changed = None" spells ghostify */
             error = PyObject_SetAttr((PyObject *)object, py__p_changed,
 				     Py_None);
-
-
-            /* unlink the placeholder */
-            placeholder.next->prev = placeholder.prev;
-            placeholder.prev->next = placeholder.next;
-
             here = placeholder.next;
-
-            if (error)
-                return -1; /* problem */
+            unlink_from_ring(&placeholder);
+            if (error < 0)
+                goto Done;
         }
         else {
             ENGINE_NOISE(".");
             here = here->next;
         }
     }
+    result = 0;
+ Done:
+    return result;
 }
 
 static PyObject *
@@ -241,7 +249,7 @@
 
     ENGINE_NOISE("<");
     self->ring_lock = 1;
-    if (scan_gc_items(self, target_size)) {
+    if (scan_gc_items(self, target_size) < 0) {
         self->ring_lock = 0;
         return NULL;
     }