Detect whether we need to do a gc after sweeping the cache. Document why we...

Detect whether we need to do a gc after sweeping the cache. Document why we would need to or not. Add tests to make sure it works (previously only in the ZODB tests.)

persistent/picklecache.py

@@ -13,6 +13,7 @@
 ##############################################################################
 import gc
 import weakref
+import sys
 
 from zope.interface import implementer
 
@@ -27,6 +28,24 @@ from persistent.persistence import _estimated_size_in_24_bits
 _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')
+
 class RingNode(object):
     # 32 byte fixed size wrapper.
     __slots__ = ('object', 'next', 'prev')
@@ -371,10 +390,8 @@ class PickleCache(object):
                     ejected += 1
                     self.__remove_from_ring(node)
             node = node.next
-        if ejected:
-            # TODO: Only do this on PyPy/Jython?
-            # Even on CPython, though, it could trigger a lot of Persistent
-            # object deallocations and dictionary mutations
+        if ejected and _SWEEP_NEEDS_GC:
+            # See comments on _SWEEP_NEEDS_GC
             gc.collect()
         return ejected
 

persistent/tests/test_picklecache.py

@@ -22,9 +22,13 @@ 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
@@ -1000,15 +1004,13 @@ class PickleCacheTests(unittest.TestCase):
             # size; if we don't get deactivated by sweeping, the cache size
             # won't shrink so this also validates that _p_deactivate gets
             # called when ejecting an object.
-            o._p_deactivate = lambda: cache.update_object_size_estimation(oid,
-                                                                          -1)
-
-
+            o._p_deactivate = lambda: cache.update_object_size_estimation(oid, -1)
         self.assertEqual(cache.cache_non_ghost_count, 100)
 
         # A GC at this point does nothing
         cache.incrgc()
         self.assertEqual(cache.cache_non_ghost_count, 100)
+        self.assertEqual(len(cache), 100)
 
         # Now if we set a byte target:
 
@@ -1024,6 +1026,10 @@ class PickleCacheTests(unittest.TestCase):
         # sanity check
         self.assertTrue(cache.total_estimated_size >= 0)
 
+        # It also shrank the measured size of the cache;
+        # this would fail under PyPy if _SWEEP_NEEDS_GC was False
+        self.assertEqual(len(cache), 1)
+
     def test_invalidate_persistent_class_calls_p_invalidate(self):
         from persistent._compat import _b
         KEY = _b('pclass')