Add a non-CFFI implementation of the Ring abstraction based on the deque work,...

Add a non-CFFI implementation of the Ring abstraction based on the deque work, and add a tox environment to test it. Only one ZODB test fails.

persistent/picklecache.py

@@ -69,9 +69,7 @@ def _sweeping_ring(f):
             self._is_sweeping_ring = False
     return locked
 
-from collections import deque
-
-from . import ring
+from .ring import Ring
 
 @implementer(IPickleCache)
 class PickleCache(object):
@@ -102,8 +100,7 @@ class PickleCache(object):
         self.non_ghost_count = 0
         self.persistent_classes = {}
         self.data = weakref.WeakValueDictionary()
-        self.ring_home = ring.CPersistentRingHead()
-        self.ring_nodes = set()
+        self.ring = Ring()
         self.cache_size_bytes = cache_size_bytes
 
     # IPickleCache API
@@ -163,11 +160,8 @@ class PickleCache(object):
         else:
             self.data[oid] = value
             _gc_monitor(value)
-            node = ring.CPersistentRing(value)
-            object.__setattr__(value, '_Persistent__ring', node)
-            if _OGA(value, '_p_state') != GHOST:
-                ring.add(self.ring_home, node)
-                self.ring_nodes.add(node)
+            if _OGA(value, '_p_state') != GHOST and value not in self.ring:
+                self.ring.add(value)
                 self.non_ghost_count += 1
 
     def __delitem__(self, oid):
@@ -179,7 +173,7 @@ class PickleCache(object):
             del self.persistent_classes[oid]
         else:
             value = self.data.pop(oid)
-            self._remove_from_ring(value)
+            self.ring.delete(value)
 
     def get(self, oid, default=None):
         """ See IPickleCache.
@@ -200,24 +194,19 @@ class PickleCache(object):
             return False # marker return for tests
 
         value = self.data[oid]
-        node = _OGA(value, '_Persistent__ring')
-        if node is None: # tests
-            return
 
-        was_in_ring = bool(node.r_next)
+        was_in_ring = value in self.ring
         if not was_in_ring:
             if _OGA(value, '_p_state') != GHOST:
-                ring.add(self.ring_home, node)
-                self.ring_nodes.add(node)
+                self.ring.add(value)
                 self.non_ghost_count += 1
         else:
-            ring.move_to_head(self.ring_home, node)
+            self.ring.move_to_head(value)
 
     def ringlen(self):
         """ See IPickleCache.
         """
-        return ring.ringlen(self.ring_home)
-
+        return len(self.ring)
 
     def items(self):
         """ See IPickleCache.
@@ -228,8 +217,7 @@ class PickleCache(object):
         """ See IPickleCache.
         """
         result = []
-        for item in ring.iteritems(self.ring_home):
-            obj = ring.get_object(item)
+        for obj in self.ring:
             result.append((obj._p_oid, obj))
         return result
 
@@ -356,15 +344,17 @@ class PickleCache(object):
 
     @_sweeping_ring
     def _sweep(self, target, target_size_bytes=0):
-        # lock
-        ejected = 0
-        for here in list(ring.iteritems(self.ring_home)):
-
+        # To avoid mutating datastructures in place or making a copy,
+        # and to work efficiently with both the CFFI ring and the
+        # deque-based ring, we collect the objects and their indexes
+        # up front and then hand them off for ejection.
+        # We don't use enumerate because that's slow under PyPy
+        i = -1
+        to_eject = []
+        for value in self.ring:
             if self.non_ghost_count <= target and (self.total_estimated_size <= target_size_bytes or not target_size_bytes):
                 break
-            value = ring.get_object(here)
-            here = here.r_next
-
+            i += 1
             if value._p_state == UPTODATE:
                 # The C implementation will only evict things that are specifically
                 # in the up-to-date state
@@ -387,9 +377,13 @@ class PickleCache(object):
                     # they don't cooperate (without this check a bunch of test_picklecache
                     # breaks)
                     or not isinstance(value, _SWEEPABLE_TYPES)):
-                    self._remove_from_ring(value)
+                    to_eject.append((i, value))
                     self.non_ghost_count -= 1
-                    ejected += 1
+
+        ejected = len(to_eject)
+        if ejected:
+            self.ring.delete_all(to_eject)
+        del to_eject # Got to clear our local if we want the GC to get the weak refs
 
         if ejected and _SWEEP_NEEDS_GC:
             # See comments on _SWEEP_NEEDS_GC
@@ -401,7 +395,7 @@ class PickleCache(object):
         value = self.data.get(oid)
         if value is not None and value._p_state != GHOST:
             value._p_invalidate()
-            self._remove_from_ring(value)
+            self.ring.delete(value)
         elif oid in self.persistent_classes:
             persistent_class = self.persistent_classes[oid]
             del self.persistent_classes[oid]
@@ -412,8 +406,3 @@ class PickleCache(object):
                 persistent_class._p_invalidate()
             except AttributeError:
                 pass
-
-    def _remove_from_ring(self, value):
-        if value._Persistent__ring.r_next:
-            ring.del_(value._Persistent__ring)
-            self.ring_nodes.discard(value._Persistent__ring)

persistent/ring.py

-#!/usr/bin/env python
 # -*- coding: utf-8 -*-
-"""
-
-
-
-"""
-from cffi import FFI
-import pkg_resources
-import os
-
-ffi = FFI()
-
-ffi.cdef("""
-typedef struct CPersistentRingEx_struct
-{
-    struct CPersistentRingEx_struct *r_prev;
-    struct CPersistentRingEx_struct *r_next;
-    void* object;
-} CPersistentRingEx;
-""")
+##############################################################################
+#
+# Copyright (c) 2009 Zope Foundation and Contributors.
+# All Rights Reserved.
+#
+# This software is subject to the provisions of the Zope Public License,
+# Version 2.1 (ZPL).  A copy of the ZPL should accompany this distribution.
+# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED
+# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS
+# FOR A PARTICULAR PURPOSE.
+#
+##############################################################################
+
+#pylint: disable=W0212
+
+try:
+    from cffi import FFI
+    import os
+
+    this_dir = os.path.dirname(os.path.abspath(__file__))
+
+    ffi = FFI()
+    with open(os.path.join(this_dir, 'ring.h')) as f:
+        ffi.cdef(f.read())
+
+    ring = ffi.verify("""
+    #include "ring.c"
+    """, include_dirs=[os.path.dirname(os.path.abspath(__file__))])
+
+    _OGA = object.__getattribute__
+    _OSA = object.__setattr__
+
+    #pylint: disable=E1101
+    class _CFFIRing(object):
+
+        __slots__ = ('ring_home', 'ring_to_obj')
+
+        def __init__(self):
+            node = self.ring_home = ffi.new("CPersistentRing*")
+            node.r_next = node
+            node.r_prev = node
+
+            self.ring_to_obj = dict()
+
+        def __len__(self):
+            return len(self.ring_to_obj)
+
+        def __contains__(self, pobj):
+            return getattr(pobj, '_Persistent__ring', self) in self.ring_to_obj
+
+        def add(self, pobj):
+            node = ffi.new("CPersistentRing*")
+            ring.ring_add(self.ring_home, node)
+            self.ring_to_obj[node] = pobj
+            object.__setattr__(pobj, '_Persistent__ring', node)
+
+        def delete(self, pobj):
+            node = getattr(pobj, '_Persistent__ring', None)
+            if node is not None and node.r_next:
+                ring.ring_del(node)
+            self.ring_to_obj.pop(node, None)
+
+        def move_to_head(self, pobj):
+            node = pobj._Persistent__ring
+            ring.ring_move_to_head(self.ring_home, node)
+
+        def delete_all(self, indexes_and_values):
+            for _, value in indexes_and_values:
+                self.delete(value)
+
+        def iteritems(self):
+            head = self.ring_home
+            here = head.r_next
+            while here != head:
+                yield here
+                here = here.r_next
 
-ffi.cdef(pkg_resources.resource_string('persistent', 'ring.h'))
+        def __iter__(self):
+            ring_to_obj = self.ring_to_obj
+            for node in self.iteritems():
+                yield ring_to_obj[node]
 
-ring = ffi.verify("""
-typedef struct CPersistentRingEx_struct
-{
-    struct CPersistentRingEx_struct *r_prev;
-    struct CPersistentRingEx_struct *r_next;
-    void* object;
-} CPersistentRingEx;
-#include "ring.c"
-""", include_dirs=[os.path.dirname(os.path.abspath(__file__))])
+    Ring = _CFFIRing
 
+except ImportError:
 
-class CPersistentRing(object):
+    from collections import deque
 
-    def __init__(self, obj=None):
-        self.handle = None
-        self.node = ffi.new("CPersistentRingEx*")
-        if obj is not None:
-            self.handle = self.node.object = ffi.new_handle(obj)
-            self._object = obj # Circular reference
+    class _DequeRing(object):
 
-    def __getattr__(self, name):
-        return getattr(self.node, name)
+        __slots__ = ('ring', 'ring_oids')
 
-    def get_object(self):
-        return get_object(self.node)
+        def __init__(self):
 
-def CPersistentRingHead():
-    head = CPersistentRing()
-    head.node.r_next = head.node
-    head.node.r_prev = head.node
-    return head
+            self.ring = deque()
+            self.ring_oids = set()
 
-def _c(node):
-    return ffi.cast("CPersistentRing*", node.node)
+        def __len__(self):
+            return len(self.ring)
 
-def add(head, elt):
-    ring.ring_add(_c(head), _c(elt))
+        def __contains__(self, pobj):
+            return pobj._p_oid in self.ring_oids
 
-def del_(elt):
-    ring.ring_del(_c(elt))
+        def add(self, pobj):
+            self.ring.append(pobj)
+            self.ring_oids.add(pobj._p_oid)
 
-def move_to_head(head, elt):
-    ring.ring_move_to_head(_c(head), _c(elt))
+        def delete(self, pobj):
+            # Note that we do not use self.ring.remove() because that
+            # uses equality semantics and we don't want to call the persistent
+            # object's __eq__ method (which might wake it up just after we
+            # tried to ghost it)
+            i = 0 # Using a manual numeric counter instead of enumerate() is much faster on PyPy
+            for o in self.ring:
+                if o is pobj:
+                    del self.ring[i]
+                    self.ring_oids.discard(pobj._p_oid)
+                    return 1
+                i += 1
 
-def iteritems(head):
-    here = head.r_next
-    while here != head.node:
-        yield here
-        here = here.r_next
+        def move_to_head(self, pobj):
+            self.delete(pobj)
+            self.add(pobj)
 
-def ringlen(head):
-    count = 0
-    for _ in iteritems(head):
-        count += 1
-    return count
+        def delete_all(self, indexes_and_values):
+            for ix, value in reversed(indexes_and_values):
+                del self.ring[ix]
+                self.ring_oids.discard(value._p_oid)
 
-def get_object(node):
-    return ffi.from_handle(node.object)
+        def __iter__(self):
+            return iter(self.ring)
 
-print CPersistentRing()
+    Ring = _DequeRing

persistent/tests/test_picklecache.py

@@ -11,9 +11,16 @@
 # FOR A PARTICULAR PURPOSE.
 #
 ##############################################################################
-import unittest
 import gc
-_is_jython = hasattr(gc, 'getJythonGCFlags')
+import os
+import platform
+import sys
+import unittest
+
+_py_impl = getattr(platform, 'python_implementation', lambda: None)
+_is_pypy = _py_impl() == 'PyPy'
+_is_jython = 'java' in sys.platform
+
 _marker = object()
 
 class PickleCacheTests(unittest.TestCase):
@@ -970,22 +977,6 @@ class PickleCacheTests(unittest.TestCase):
         finally:
             persistent.picklecache._SWEEPABLE_TYPES = sweep_types
 
-    def test_invalidate_not_in_cache(self):
-        # A contrived test of corruption
-        cache = self._makeOne()
-        p = self._makePersist(jar=cache.jar)
-
-        p._p_state = 0 # non-ghost, get in the ring
-        cache[p._p_oid] = p
-        self.assertEqual(cache.cache_non_ghost_count, 1)
-
-        from ..ring import get_object,ffi
-        self.assertEqual(get_object(cache.ring_home.r_next), p)
-        cache.ring_home.r_next.object = ffi.NULL
-
-        # Nothing to test, just that it doesn't break
-        cache._invalidate(p._p_oid)
-
     if _is_jython:
         def with_deterministic_gc(f):
             def test(self):
@@ -1066,6 +1057,14 @@ class PickleCacheTests(unittest.TestCase):
 
         self.assertTrue(pclass.invalidated)
 
+    def test_ring_impl(self):
+        from .. import ring
+
+        if _is_pypy or os.getenv('USING_CFFI'):
+            self.assertEqual(ring.Ring, ring._CFFIRing)
+        else:
+            self.assertEqual(ring.Ring, ring._DequeRing)
+
 class DummyPersistent(object):
 
     def _p_invalidate(self):

tox.ini

@@ -3,7 +3,7 @@ envlist =
 # Jython 2.7rc2 does work, but unfortunately has an issue running
 # with Tox 1.9.2 (http://bugs.jython.org/issue2325)
 #   py26,py27,py27-pure,pypy,py32,py33,py34,pypy3,jython,coverage,docs
-    py26,py27,py27-pure,pypy,py32,py33,py34,pypy3,coverage,docs
+    py26,py27,py27-pure,py27-pure-cffi,pypy,py32,py33,py34,pypy3,coverage,docs
 
 [testenv]
 deps =
@@ -25,6 +25,19 @@ deps =
 commands =
     python setup.py test -q
 
+[testenv:py27-pure-cffi]
+basepython =
+    python2.7
+setenv =
+    PURE_PYTHON = 1
+	USING_CFFI = 1
+deps =
+    {[testenv]deps}
+	cffi
+commands =
+    python setup.py test -q
+
+
 [testenv:coverage]
 basepython =
     python2.6