Merge pull request #107 from zopefoundation/issue77

Require CFFI on CPython for pure-Python operation.

CHANGES.rst

@@ -16,10 +16,18 @@
   - The Python implementation raises ``AttributeError`` if a
     persistent class doesn't have a ``p_jar`` attribute.
 
+  See `issue 102
+  <https://github.com/zopefoundation/persistent/issues/102>`_.
+
 - Allow sweeping cache without ``cache_size``. ``cache_size_bytes``
   works with ``cache_size=0``, no need to set ``cache_size`` to a
   large value.
 
+- Require ``CFFI`` on CPython for pure-Python operation. This drops
+  support for Jython (which was untested). See `issue 77
+  <https://github.com/zopefoundation/persistent/issues/77>`_.
+
+
 4.4.3 (2018-10-22)
 ------------------
 

persistent/ring.py

@@ -13,11 +13,13 @@
 #
 ##############################################################################
 
-#pylint: disable=W0212,E0211,W0622,E0213,W0221,E0239
+# pylint:disable=inherit-non-class,no-self-argument,redefined-builtin,c-extension-no-member
 
 from zope.interface import Interface
 from zope.interface import implementer
 
+from persistent import _ring
+
 class IRing(Interface):
     """Conceptually, a doubly-linked list for efficiently keeping track of least-
     and most-recently used :class:`persistent.interfaces.IPersistent` objects.
@@ -28,13 +30,13 @@ class IRing(Interface):
     explaining assumptions and performance requirements.
     """
 
-    def __len__():
+    def __len__(): # pylint:disable=no-method-argument
         """Return the number of persistent objects stored in the ring.
 
         Should be constant time.
         """
 
-    def __contains__(object):
+    def __contains__(object): # pylint:disable=unexpected-special-method-signature
         """Answer whether the given persistent object is found in the ring.
 
         Must not rely on object equality or object hashing, but only
@@ -83,7 +85,7 @@ class IRing(Interface):
         Should be at least linear time (not quadratic).
         """
 
-    def __iter__():
+    def __iter__(): # pylint:disable=no-method-argument
         """Iterate over each persistent object in the ring, in the order of least
         recently used to most recently used.
 
@@ -91,127 +93,72 @@ class IRing(Interface):
         undefined consequences.
         """
 
-from collections import deque
 
-@implementer(IRing)
-class _DequeRing(object):
-    """A ring backed by the :class:`collections.deque` class.
+ffi = _ring.ffi
+_FFI_RING = _ring.lib
 
-    Operations are a mix of constant and linear time.
+_OGA = object.__getattribute__
+_OSA = object.__setattr__
 
-    It is available on all platforms.
+
+@implementer(IRing)
+class _CFFIRing(object):
+    """A ring backed by a C implementation. All operations are constant time.
+
+    It is only available on platforms with ``cffi`` installed.
     """
 
-    __slots__ = ('ring', 'ring_oids')
+    __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 = deque()
-        self.ring_oids = set()
+        # In order for the CFFI objects to stay alive, we must keep
+        # a strong reference to them, otherwise they get freed. We must
+        # also keep strong references to the objects so they can be deactivated
+        self.ring_to_obj = dict()
 
     def __len__(self):
-        return len(self.ring)
+        return len(self.ring_to_obj)
 
     def __contains__(self, pobj):
-        return pobj._p_oid in self.ring_oids
+        return getattr(pobj, '_Persistent__ring', self) in self.ring_to_obj
 
     def add(self, pobj):
-        self.ring.append(pobj)
-        self.ring_oids.add(pobj._p_oid)
+        node = ffi.new("CPersistentRing*")
+        _FFI_RING.ring_add(self.ring_home, node)
+        self.ring_to_obj[node] = pobj
+        _OSA(pobj, '_Persistent__ring', node)
 
     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)
-        for i, o in enumerate(self.ring):
-            if o is pobj:
-                del self.ring[i]
-                self.ring_oids.discard(pobj._p_oid)
-                return 1
+        its_node = getattr(pobj, '_Persistent__ring', None)
+        our_obj = self.ring_to_obj.pop(its_node, None)
+        if its_node is not None and our_obj is not None and its_node.r_next:
+            _FFI_RING.ring_del(its_node)
+            return 1
+        return None
 
     def move_to_head(self, pobj):
-        self.delete(pobj)
-        self.add(pobj)
+        node = _OGA(pobj, '_Persistent__ring')
+        _FFI_RING.ring_move_to_head(self.ring_home, node)
 
     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 __iter__(self):
-        return iter(self.ring)
+        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
 
-try:
-    from persistent import _ring
-except ImportError: # pragma: no cover
-    _CFFIRing = None
-else:
-    ffi = _ring.ffi
-    _FFI_RING = _ring.lib
-
-    _OGA = object.__getattribute__
-    _OSA = object.__setattr__
-
-    #pylint: disable=E1101
-    @implementer(IRing)
-    class _CFFIRing(object):
-        """A ring backed by a C implementation. All operations are constant time.
-
-        It is only available on platforms with ``cffi`` installed.
-        """
-
-        __slots__ = ('ring_home', 'ring_to_obj')
-
-        def __init__(self):
-            node = self.ring_home = ffi.new("CPersistentRing*")
-            node.r_next = node
-            node.r_prev = node
-
-            # In order for the CFFI objects to stay alive, we must keep
-            # a strong reference to them, otherwise they get freed. We must
-            # also keep strong references to the objects so they can be deactivated
-            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*")
-            _FFI_RING.ring_add(self.ring_home, node)
-            self.ring_to_obj[node] = pobj
-            _OSA(pobj, '_Persistent__ring', node)
-
-        def delete(self, pobj):
-            its_node = getattr(pobj, '_Persistent__ring', None)
-            our_obj = self.ring_to_obj.pop(its_node, None)
-            if its_node is not None and our_obj is not None and its_node.r_next:
-                _FFI_RING.ring_del(its_node)
-                return 1
-
-        def move_to_head(self, pobj):
-            node = _OGA(pobj, '_Persistent__ring')
-            _FFI_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
-
-        def __iter__(self):
-            ring_to_obj = self.ring_to_obj
-            for node in self.iteritems():
-                yield ring_to_obj[node]
+    def __iter__(self):
+        ring_to_obj = self.ring_to_obj
+        for node in self.iteritems():
+            yield ring_to_obj[node]
 
 # Export the best available implementation
-Ring = _CFFIRing if _CFFIRing else _DequeRing
+Ring = _CFFIRing

persistent/tests/test_persistence.py

@@ -22,7 +22,6 @@ from persistent._compat import copy_reg
 
 
 _is_pypy3 = platform.python_implementation() == 'PyPy' and sys.version_info[0] > 2
-_is_jython = platform.python_implementation() == 'Jython'
 
 # pylint:disable=R0904,W0212,E1101
 # pylint:disable=attribute-defined-outside-init,too-many-lines
@@ -985,7 +984,7 @@ class _Persistent_Base(object):
         self.assertEqual(inst.baz, 'bam')
         self.assertEqual(inst.qux, 'spam')
 
-    if not _is_pypy3 and not _is_jython:
+    if not _is_pypy3:
         def test___setstate___interns_dict_keys(self):
             class Derived(self._getTargetClass()):
                 pass

persistent/tests/test_picklecache.py

@@ -12,9 +12,7 @@
 #
 ##############################################################################
 import gc
-import os
 import platform
-import sys
 import unittest
 
 from persistent.interfaces import UPTODATE
@@ -22,7 +20,6 @@ from persistent.interfaces import UPTODATE
 # pylint:disable=protected-access,too-many-lines,too-many-public-methods
 
 _is_pypy = platform.python_implementation() == 'PyPy'
-_is_jython = 'java' in sys.platform
 
 _marker = object()
 
@@ -32,22 +29,12 @@ def skipIfNoCExtension(o):
         persistent._cPickleCache is None,
         "The C extension is not available")(o)
 
-
-if _is_jython: # pragma: no cover
-    def with_deterministic_gc(f):
-        def test(self):
-            # pylint:disable=no-member
-            old_flags = gc.getMonitorGlobal()
-            gc.setMonitorGlobal(True)
-            try:
-                f(self, force_collect=True)
-            finally:
-                gc.setMonitorGlobal(old_flags)
-        return test
-else:
-    def with_deterministic_gc(f):
-        return f
-
+def skipIfPurePython(o):
+    import persistent._compat
+    return unittest.skipIf(
+        persistent._compat.PURE_PYTHON,
+        "Cannot mix and match implementations"
+    )(o)
 
 class PickleCacheTests(unittest.TestCase):
 
@@ -657,14 +644,6 @@ class PickleCacheTests(unittest.TestCase):
 
         self.assertTrue(pclass.invalidated)
 
-    def test_ring_impl(self):
-        from .. import ring
-
-        expected = (ring._CFFIRing
-                    if _is_pypy or ring._CFFIRing is not None or os.environ.get('USING_CFFI')
-                    else ring._DequeRing)
-        self.assertIs(ring.Ring, expected)
-
 
 class PythonPickleCacheTests(PickleCacheTests):
     # Tests that depend on the implementation details of the
@@ -1007,9 +986,8 @@ class PythonPickleCacheTests(PickleCacheTests):
         self.assertTrue(items[0][1] is candidate)
         self.assertEqual(candidate._p_state, UPTODATE)
 
-    @with_deterministic_gc
     def test_cache_garbage_collection_bytes_also_deactivates_object(self,
-                                                                    force_collect=_is_pypy or _is_jython):
+                                                                    force_collect=_is_pypy):
 
         class MyPersistent(self._getDummyPersistentClass()):
             def _p_deactivate(self):
@@ -1070,9 +1048,7 @@ class PythonPickleCacheTests(PickleCacheTests):
         candidate._p_jar = None
         self.assertRaises(KeyError, cache.new_ghost, key, candidate)
 
-    @with_deterministic_gc
-    def test_cache_garbage_collection_bytes_with_cache_size_0(
-      self, force_collect=_is_pypy or _is_jython):
+    def test_cache_garbage_collection_bytes_with_cache_size_0(self):
 
         class MyPersistent(self._getDummyPersistentClass()):
             def _p_deactivate(self):
@@ -1116,6 +1092,7 @@ class PythonPickleCacheTests(PickleCacheTests):
 
 
 @skipIfNoCExtension
+@skipIfPurePython
 class CPickleCacheTests(PickleCacheTests):
 
     def _getTargetClass(self):

persistent/tests/test_ring.py

@@ -15,7 +15,7 @@ import unittest
 
 from .. import ring
 
-#pylint: disable=R0904,W0212,E1101
+# pylint:disable=protected-access
 
 class DummyPersistent(object):
     _p_oid = None
@@ -34,11 +34,11 @@ class DummyPersistent(object):
     def __repr__(self): # pragma: no cover
         return "<Dummy %r>" % self._p_oid
 
-class _Ring_Base(object):
+
+class CFFIRingTests(unittest.TestCase):
 
     def _getTargetClass(self):
-        """Return the type of the ring to test"""
-        raise NotImplementedError()
+        return ring._CFFIRing
 
     def _makeOne(self):
         return self._getTargetClass()()
@@ -137,21 +137,3 @@ class _Ring_Base(object):
         r.delete_all([(0, p1), (2, p3)])
         self.assertEqual([p2], list(r))
         self.assertEqual(1, len(r))
-
-class DequeRingTests(unittest.TestCase, _Ring_Base):
-
-    def _getTargetClass(self):
-        return ring._DequeRing
-
-_add_to_suite = [DequeRingTests]
-
-if ring._CFFIRing:
-    class CFFIRingTests(unittest.TestCase, _Ring_Base):
-
-        def _getTargetClass(self):
-            return ring._CFFIRing
-
-    _add_to_suite.append(CFFIRingTests)
-
-def test_suite():
-    return unittest.TestSuite([unittest.makeSuite(x) for x in _add_to_suite])

persistent/tests/test_timestamp.py

@@ -232,12 +232,6 @@ class PyAndCComparisonTests(unittest.TestCase):
         from persistent.timestamp import pyTimeStamp
         return pyTimeStamp(*args, **kwargs)
 
-    @property
-    def _is_jython(self):
-        import platform
-        py_impl = getattr(platform, 'python_implementation', lambda: None)
-        return py_impl() == 'Jython'
-
     def _make_C_and_Py(self, *args, **kwargs):
         return self._makeC(*args, **kwargs), self._makePy(*args, **kwargs)
 
@@ -300,18 +294,7 @@ class PyAndCComparisonTests(unittest.TestCase):
             MUT.c_long = c_int64
             # call __hash__ directly to avoid interpreter truncation
             # in hash() on 32-bit platforms
-            if not self._is_jython:
-                self.assertEqual(py.__hash__(), bit_64_hash)
-            else: # pragma: no cover
-                # Jython 2.7's ctypes module doesn't properly
-                # implement the 'value' attribute by truncating.
-                # (It does for native calls, but not visibly to Python).
-                # Therefore we get back the full python long. The actual
-                # hash() calls are correct, though, because the JVM uses
-                # 32-bit ints for its hashCode methods.
-                self.assertEqual(
-                    py.__hash__(),
-                    384009219096809580920179179233996861765753210540033)
+            self.assertEqual(py.__hash__(), bit_64_hash)
         finally:
             MUT._MAXINT = orig_maxint
             if orig_c_long is not None:
@@ -320,7 +303,7 @@ class PyAndCComparisonTests(unittest.TestCase):
                 del MUT.c_long
 
         # These are *usually* aliases, but aren't required
-        # to be (and aren't under Jython 2.7).
+        # to be
         expected_hash = bit_32_hash if is_32_bit_hash else bit_64_hash
         self.assertEqual(py.__hash__(), expected_hash)
 
@@ -328,9 +311,9 @@ class PyAndCComparisonTests(unittest.TestCase):
         # The simple constants make it easier to diagnose
         # a difference in algorithms
         import persistent.timestamp as MUT
-        # We get 32-bit hash values on 32-bit platforms, or on the JVM
+        # We get 32-bit hash values on 32-bit platforms,
         # OR on Windows (whether compiled in 64 or 32-bit mode)
-        is_32_bit = MUT._MAXINT == (2**31 - 1) or self._is_jython or sys.platform == 'win32'
+        is_32_bit = MUT._MAXINT == (2**31 - 1) or sys.platform == 'win32'
 
         c, py = self._make_C_and_Py(b'\x00\x00\x00\x00\x00\x00\x00\x00')
         self.assertEqual(hash(c), 8)

setup.py

@@ -20,7 +20,7 @@ from setuptools import Extension
 from setuptools import find_packages
 from setuptools import setup
 
-version = '4.4.4.dev0'
+version = '4.5.0.dev0'
 
 here = os.path.abspath(os.path.dirname(__file__))
 
@@ -33,12 +33,12 @@ def _read_file(filename):
 README = (_read_file('README.rst') + '\n\n' + _read_file('CHANGES.rst'))
 
 is_pypy = platform.python_implementation() == 'PyPy'
-is_jython = 'java' in sys.platform
 
-# Jython cannot build the C optimizations, while on PyPy they are
+# On PyPy the C optimizations are
 # anti-optimizations (the C extension compatibility layer is known-slow,
-# and defeats JIT opportunities).
-if is_pypy or is_jython:
+# and defeats JIT opportunities); PyPy 6.0 can compile them, but the
+# tests fail and they actually crash the VM.
+if is_pypy:
     # Note that all the lists we pass to setuptools must be distinct
     # objects, or bad things happen. See https://github.com/zopefoundation/persistent/issues/88
     ext_modules = []
@@ -120,7 +120,6 @@ setup(name='persistent',
       extras_require={
           'test': [
               'zope.testrunner',
-              "cffi ; platform_python_implementation == 'CPython'",
               'manuel',
           ],
           'testing': (),
@@ -131,6 +130,10 @@ setup(name='persistent',
       },
       install_requires=[
           'zope.interface',
+          "cffi ; platform_python_implementation == 'CPython'",
+      ],
+      setup_requires=[
+          "cffi ; platform_python_implementation == 'CPython'",
       ],
       entry_points={},
 )

tox.ini

 [tox]
 envlist =
-# Jython 2.7rc2 does work, but unfortunately has an issue running
-# with Tox 1.9.2 (http://bugs.jython.org/issue2325)
-#   py27,py27-pure,pypy,py33,py34,pypy3,jython,coverage,docs
     py27,py27-pure,py27-pure-cffi,pypy,py34,py35,py36,py37,pypy3,coverage,docs
 
 [testenv]
 deps =
-     cffi
     .[test]
 commands =
     zope-testrunner --test-path=.
@@ -23,7 +19,6 @@ basepython =
     python2.7
 setenv =
     PURE_PYTHON = 1
-    USING_CFFI = 1
 
 [testenv:coverage]
 usedevelop = true
@@ -35,8 +30,6 @@ commands =
 deps =
     {[testenv]deps}
     coverage
-setenv =
-    USING_CFFI = 1
 
 [testenv:docs]
 basepython =