Merge pull request #54 from zopefoundation/new_ghost_doesnt_clear_newargs

new_ghost shouldn't clear newargs (PyPy consistent with C)

.gitignore

@@ -15,3 +15,4 @@ coverage.xml
 .installed.cfg
 .dir-locals.el
 dist
+htmlcov

CHANGES.rst

@@ -7,6 +7,18 @@
 - Fix the hashcode of Python ``TimeStamp`` objects on 64-bit Python on
   Windows. See https://github.com/zopefoundation/persistent/pull/55
 
+- Stop calling ``gc.collect`` every time ``PickleCache.incrgc`` is called (every
+  transaction boundary) in pure-Python mode (PyPy). This means that
+  the reported size of the cache may be wrong (until the next GC), but
+  it is much faster. This should not have any observable effects for
+  user code.
+
+- Stop clearing the dict and slots of objects added to
+  ``PickleCache.new_ghost`` (typically these values are passed to
+  ``__new__`` from the pickle data) in pure-Python mode (PyPy). This
+  matches the behaviour of the C code.
+
+
 4.2.2 (2016-11-29)
 ------------------
 

persistent/persistence.py

@@ -413,19 +413,24 @@ class Persistent(object):
         _OSA(self, '_Persistent__flags', 0)
         self._p_deactivate()
 
-    def _p_invalidate_deactivate_helper(self):
+    def _p_invalidate_deactivate_helper(self, clear=True):
         jar = _OGA(self, '_Persistent__jar')
         if jar is None:
             return
 
         if _OGA(self, '_Persistent__flags') is not None:
             _OSA(self, '_Persistent__flags', None)
-        idict = getattr(self, '__dict__', None)
-        if idict is not None:
+
+        if clear:
+            try:
+                idict = _OGA(self, '__dict__')
+            except AttributeError:
+                pass
+            else:
                 idict.clear()
             type_ = type(self)
-        # ( for backward-compatibility reason we release __slots__ only if
-        #   class does not override __new__ )
+            # for backward-compatibility reason we release __slots__ only if
+            # class does not override __new__
             if type_.__new__ is Persistent.__new__:
                 for slotname in Persistent._slotnames(self, _v_exclude=False):
                     try:

persistent/picklecache.py

@@ -13,7 +13,7 @@
 ##############################################################################
 import gc
 import weakref
-import sys
+
 
 from zope.interface import implementer
 
@@ -21,31 +21,13 @@ from persistent.interfaces import GHOST
 from persistent.interfaces import IPickleCache
 from persistent.interfaces import OID_TYPE
 from persistent.interfaces import UPTODATE
-from persistent import Persistent
+from persistent.persistence import Persistent
 from persistent.persistence import _estimated_size_in_24_bits
 
 # Tests may modify this to add additional types
 _CACHEABLE_TYPES = (type, Persistent)
 _SWEEPABLE_TYPES = (Persistent,)
 
-# The Python PickleCache implementation keeps track of the objects it
-# is caching in a WeakValueDictionary. The number of objects in the
-# cache (in this dictionary) is exposed as the len of the cache. Under
-# non-refcounted implementations like PyPy, the weak references in
-# this dictionary are only cleared when the garbage collector runs.
-# Thus, after an incrgc, the len of the cache is incorrect for some
-# period of time unless we ask the GC to run.
-# Furthermore, evicted objects can stay in the dictionary and be returned
-# from __getitem__ or possibly conflict with a new item in __setitem__.
-# We determine whether or not we need to do the GC based on the ability
-# to get a reference count: PyPy and Jython don't use refcounts and don't
-# expose this; this is safer than blacklisting specific platforms (e.g.,
-# what about IronPython?). On refcounted platforms, we don't want to
-# run a GC to avoid possible performance regressions (e.g., it may
-# block all threads).
-# Tests may modify this
-_SWEEP_NEEDS_GC = not hasattr(sys, 'getrefcount')
-
 # On Jython, we need to explicitly ask it to monitor
 # objects if we want a more deterministic GC
 if hasattr(gc, 'monitorObject'): # pragma: no cover
@@ -264,7 +246,7 @@ class PickleCache(object):
                 # careful to avoid broken _p_invalidate and _p_deactivate
                 # that don't call the super class. See ZODB's
                 # testConnection.doctest_proper_ghost_initialization_with_empty__p_deactivate
-                obj._p_invalidate_deactivate_helper()
+                obj._p_invalidate_deactivate_helper(False)
         self[oid] = obj
 
     def reify(self, to_reify):
@@ -381,11 +363,7 @@ class PickleCache(object):
         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
-            gc.collect()
         return ejected
 
     @_sweeping_ring

persistent/tests/test_persistence.py

@@ -32,6 +32,9 @@ class _Persistent_Base(object):
         # concrete testcase classes must override
         raise NotImplementedError()
 
+    def _makeRealCache(self, jar):
+        return self._makeCache(jar)
+
     def _makeOne(self, *args, **kw):
         return self._getTargetClass()(*args, **kw)
 
@@ -1599,6 +1602,51 @@ class _Persistent_Base(object):
         p._p_invalidate()
         self.assertTrue(p.deactivated)
 
+
+    def test_new_ghost_success_not_already_ghost_dict(self):
+        # https://github.com/zopefoundation/persistent/issues/49
+        # calling new_ghost on an object that already has state just changes
+        # its flags, it doesn't destroy the state.
+        from persistent.interfaces import GHOST
+        from persistent.interfaces import UPTODATE
+        class TestPersistent(self._getTargetClass()):
+            pass
+        KEY = b'123'
+        jar = self._makeJar()
+        cache = self._makeRealCache(jar)
+        candidate = TestPersistent()
+
+        candidate.set_by_new = 1
+        self.assertEqual(candidate._p_state, UPTODATE)
+        cache.new_ghost(KEY, candidate)
+
+        self.assertTrue(cache.get(KEY) is candidate)
+        self.assertEqual(candidate._p_oid, KEY)
+        self.assertEqual(candidate._p_state, GHOST)
+        self.assertEqual(candidate.set_by_new, 1)
+
+    def test_new_ghost_success_not_already_ghost_slot(self):
+        # https://github.com/zopefoundation/persistent/issues/49
+        # calling new_ghost on an object that already has state just changes
+        # its flags, it doesn't destroy the state.
+        from persistent.interfaces import GHOST
+        from persistent.interfaces import UPTODATE
+        class TestPersistent(self._getTargetClass()):
+            __slots__ = ('set_by_new', '__weakref__')
+        KEY = b'123'
+        jar = self._makeJar()
+        cache = self._makeRealCache(jar)
+        candidate = TestPersistent()
+        candidate.set_by_new = 1
+        self.assertEqual(candidate._p_state, UPTODATE)
+        cache.new_ghost(KEY, candidate)
+
+        self.assertTrue(cache.get(KEY) is candidate)
+        self.assertEqual(candidate._p_oid, KEY)
+        self.assertEqual(candidate._p_state, GHOST)
+        self.assertEqual(candidate.set_by_new, 1)
+
+
 class PyPersistentTests(unittest.TestCase, _Persistent_Base):
 
     def _getTargetClass(self):
@@ -1627,6 +1675,10 @@ class PyPersistentTests(unittest.TestCase, _Persistent_Base):
 
         return _Cache(jar)
 
+    def _makeRealCache(self, jar):
+        from persistent.picklecache import PickleCache
+        return PickleCache(jar, 10)
+
     def _checkMRU(self, jar, value):
         self.assertEqual(list(jar._cache._mru), value)
 

persistent/tests/test_picklecache.py

@@ -30,13 +30,9 @@ class PickleCacheTests(unittest.TestCase):
         self.orig_types = persistent.picklecache._CACHEABLE_TYPES
         persistent.picklecache._CACHEABLE_TYPES += (DummyPersistent,)
 
-        self.orig_sweep_gc = persistent.picklecache._SWEEP_NEEDS_GC
-        persistent.picklecache._SWEEP_NEEDS_GC = True # coverage
-
     def tearDown(self):
         import persistent.picklecache
         persistent.picklecache._CACHEABLE_TYPES = self.orig_types
-        persistent.picklecache._SWEEP_NEEDS_GC = self.orig_sweep_gc
 
     def _getTargetClass(self):
         from persistent.picklecache import PickleCache
@@ -992,7 +988,7 @@ class PickleCacheTests(unittest.TestCase):
             return f
 
     @with_deterministic_gc
-    def test_cache_garbage_collection_bytes_also_deactivates_object(self, force_collect=False):
+    def test_cache_garbage_collection_bytes_also_deactivates_object(self, force_collect=_is_pypy or _is_jython):
         from persistent.interfaces import UPTODATE
         from persistent._compat import _b
         cache = self._makeOne()
@@ -1070,7 +1066,9 @@ class DummyPersistent(object):
         self._p_state = GHOST
 
     _p_deactivate = _p_invalidate
-    _p_invalidate_deactivate_helper = _p_invalidate
+
+    def _p_invalidate_deactivate_helper(self, clear=True):
+        self._p_invalidate()
 
     def _p_activate(self):
         from persistent.interfaces import UPTODATE