Skip to content

Commit

Permalink
Stop calling gc.collect in the Python incrgc.
Browse files Browse the repository at this point in the history
@jimfulton and I have talked about it, and I'm (mostly :) convinced that
this shouldn't be an actual problem for any of the reasons described in
the previous comment.

If I use PyPy 5.4.1 to run the ZODB master test suite against
this (well, with #44 rolled back) I don't get any unexpected
failures. (I haven't run the ZEO test suite yet.) Which honestly amazes
me because I'm sure I used to get test failures---I guess the PyPy GC
has changed...which means we may see some failures on Travis. I'll try
to set up an older PyPy to verify.
  • Loading branch information
jamadden committed Oct 27, 2016
1 parent 69633df commit 8a09a43
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 28 deletions.
6 changes: 6 additions & 0 deletions CHANGES.rst
Expand Up @@ -4,6 +4,12 @@
4.2.2 (unreleased)
------------------

- 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.

- Drop use of ``ctypes`` for determining maximum integer size, to increase
pure-Python compatibility.

Expand Down
24 changes: 1 addition & 23 deletions persistent/picklecache.py
Expand Up @@ -13,7 +13,7 @@
##############################################################################
import gc
import weakref
import sys


from zope.interface import implementer

Expand All @@ -28,24 +28,6 @@
_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
Expand Down Expand Up @@ -381,11 +363,7 @@ def _sweep(self, target, target_size_bytes=0):
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
Expand Down
6 changes: 1 addition & 5 deletions persistent/tests/test_picklecache.py
Expand Up @@ -30,13 +30,9 @@ def setUp(self):
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
Expand Down Expand Up @@ -992,7 +988,7 @@ def with_deterministic_gc(f):
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()
Expand Down

0 comments on commit 8a09a43

Please sign in to comment.