Author: Antonio Cuni <anto.c...@gmail.com>
Branch: identity-dict-strategy
Changeset: r45805:76c609d60ebd
Date: 2011-07-21 12:11 +0200
http://bitbucket.org/pypy/pypy/changeset/76c609d60ebd/
Log: kill the global versioning logic, and add a big comment which
explains why it's not needed
diff --git a/pypy/objspace/std/identitydict.py
b/pypy/objspace/std/identitydict.py
--- a/pypy/objspace/std/identitydict.py
+++ b/pypy/objspace/std/identitydict.py
@@ -1,3 +1,6 @@
+## ----------------------------------------------------------------------------
+## dict strategy (see dict_multiobject.py)
+
from pypy.rlib import rerased
from pypy.objspace.std.dictmultiobject import (AbstractTypedStrategy,
DictStrategy,
@@ -5,39 +8,6 @@
_UnwrappedIteratorMixin)
-# a global (per-space) version counter to track live instances which "compare
-# by identity" (i.e., whose __eq__, __cmp__ and __hash__ are the default
-# ones). The idea is to track only classes for which we checked the
-# compares_by_identity() status at least once: we increment the version if its
-# status might change, e.g. because we set one of those attributes. The
-# actual work is done by W_TypeObject.mutated() and objecttype:descr_setclass
-
-def bump_global_version(space):
- if space.config.objspace.std.withidentitydict:
- space.fromcache(ComparesByIdentityVersion).bump()
-
-def get_global_version(space):
- if space.config.objspace.std.withidentitydict:
- return space.fromcache(ComparesByIdentityVersion).get()
- return None
-
-class ComparesByIdentityVersion(object):
-
- def __init__(self, space):
- self.bump()
-
- def bump(self):
- from pypy.objspace.std.typeobject import VersionTag
- self._version = VersionTag()
-
- def get(self):
- return self._version
-
-
-
-## ----------------------------------------------------------------------------
-## dict strategy (see dict_multiobject.py)
-
# this strategy is selected by EmptyDictStrategy.switch_to_correct_strategy
class IdentityDictStrategy(AbstractTypedStrategy, DictStrategy):
"""
@@ -45,11 +15,48 @@
default unless you override __hash__, __eq__ or __cmp__). The storage is
just a normal RPython dict, which has already the correct by-identity
semantics.
+
+ Note that at a first sight, you might have problems if you mutate the
+ class of an object which is already inside an identitydict. Consider this
+ example::
+
+ class X(object):
+ pass
+ d = {x(): 1}
+ X.__eq__ = ...
+ d[y] # might trigger a call to __eq__?
+
+ We want to be sure that x.__eq__ is called in the same cases as in
+ CPython. However, as long as the strategy is IdentityDictStrategy, the
+ __eq__ will never be called.
+
+ It turns out that it's not a problem. In CPython (and in PyPy without
+ this strategy), the __eq__ is called if ``hash(y) == hash(x)`` and ``x is
+ not y``. Note that hash(x) is computed at the time when we insert x in
+ the dict, not at the time we lookup y.
+
+ Now, how can hash(y) == hash(x)? There are two possibilities:
+
+ 1. we write a custom __hash__ for the class of y, thus making it a not
+ "compares by reference" type
+
+ 2. the class of y is "compares by reference" type, and by chance the
+ hash is the same as x
+
+ In the first case, the getitem immediately notice that y is not of the
+ right type, and switches the strategy to ObjectDictStrategy, then the
+ lookup works as usual.
+
+ The second case is completely non-deterministic, even in CPython.
+ Depending on the phase of the moon, you might call the __eq__ or not, so
+ it is perfectly fine to *never* call it. Morever, in practice with the
+ minimar GC we never have two live objects with the same hash, so it would
+ never happen anyway.
"""
- _erase_tuple, _unerase_tuple = rerased.new_erasing_pair("identitydict")
- _erase_tuple = staticmethod(_erase_tuple)
- _unerase_tuple = staticmethod(_unerase_tuple)
+ erase, unerase = rerased.new_erasing_pair("identitydict")
+ erase = staticmethod(erase)
+ unerase = staticmethod(unerase)
def wrap(self, unwrapped):
return unwrapped
@@ -57,18 +64,6 @@
def unwrap(self, wrapped):
return wrapped
- def erase(self, d):
- current_version = get_global_version(self.space)
- return self._erase_tuple((current_version, d))
-
- def unerase(self, dstorage):
- version, d = self._unerase_tuple(dstorage)
- return d
-
- def get_current_version(self, dstorage):
- version, d = self._unerase_tuple(dstorage)
- return version
-
def get_empty_storage(self):
return self.erase({})
diff --git a/pypy/objspace/std/objecttype.py b/pypy/objspace/std/objecttype.py
--- a/pypy/objspace/std/objecttype.py
+++ b/pypy/objspace/std/objecttype.py
@@ -43,9 +43,6 @@
assert isinstance(w_oldcls, W_TypeObject)
if w_oldcls.get_full_instance_layout() ==
w_newcls.get_full_instance_layout():
w_obj.setclass(space, w_newcls)
- if space.config.objspace.std.withidentitydict:
- if w_oldcls.compares_by_identity() and not
w_newcls.compares_by_identity():
- identitydict.bump_global_version(space)
else:
raise operationerrfmt(space.w_TypeError,
"__class__ assignment: '%s' object layout
differs from '%s'",
diff --git a/pypy/objspace/std/test/test_identitydict.py
b/pypy/objspace/std/test/test_identitydict.py
--- a/pypy/objspace/std/test/test_identitydict.py
+++ b/pypy/objspace/std/test/test_identitydict.py
@@ -2,7 +2,7 @@
from pypy.conftest import gettestobjspace
from pypy.conftest import option
-class AppTestTrackVersion:
+class AppTestComparesByIdentity:
def setup_class(cls):
from pypy.objspace.std import identitydict
@@ -13,15 +13,6 @@
return space.wrap(w_cls.compares_by_identity())
cls.w_compares_by_identity =
cls.space.wrap(interp2app(compares_by_identity))
- def get_version(space):
- v = cls.versions.setdefault(identitydict.get_global_version(space),
- len(cls.versions))
- return space.wrap(v)
- cls.w_get_version = cls.space.wrap(interp2app(get_version))
-
- def setup_method(self, m):
- self.__class__.versions = {}
-
def test_compares_by_identity(self):
class Plain(object):
pass
@@ -53,56 +44,6 @@
del X.__eq__
assert self.compares_by_identity(X)
- def test_versioning(self):
- class X(object):
- pass
-
- class Y(object):
- def __eq__(self, other):
- pass
-
- assert self.get_version() == 0
- X.__eq__ = lambda x: None
- # modifying a class for which we never checked the
- # compares_by_identity() status does not increase the version
- assert self.get_version() == 0
-
- del X.__eq__
- assert self.compares_by_identity(X) # now we check it
- X.__add__ = lambda x: None
- assert self.get_version() == 0 # innocent change
- #
- X.__eq__ = lambda x: None
- assert self.get_version() == 1 # BUMP!
-
- del X.__eq__
- assert self.compares_by_identity(X)
- X.__bases__ = (object,)
- assert self.get_version() == 2 # BUMP!
-
- # modifying a class which is already "bad" does not increase the
- # version
- Y.__eq__ = lambda x: None
- assert self.get_version() == 2
-
- def test_change___class__(self):
- class X(object):
- pass
-
- class Y(object):
- pass
-
- class Z(object):
- def __eq__(self, other):
- pass
-
- x = X()
- assert self.compares_by_identity(X)
- assert self.get_version() == 0
- x.__class__ = Y
- assert self.get_version() == 0
- x.__class__ = Z
- assert self.get_version() == 1
class AppTestIdentityDict(object):
diff --git a/pypy/objspace/std/typeobject.py b/pypy/objspace/std/typeobject.py
--- a/pypy/objspace/std/typeobject.py
+++ b/pypy/objspace/std/typeobject.py
@@ -178,8 +178,6 @@
if (key is None or key == '__eq__' or
key == '__cmp__' or key == '__hash__'):
w_self.compares_by_identity_status = UNKNOWN
- if did_compare_by_identity:
- identitydict.bump_global_version(w_self.space)
if space.config.objspace.std.newshortcut:
w_self.w_bltin_new = None
_______________________________________________
pypy-commit mailing list
pypy-commit@python.org
http://mail.python.org/mailman/listinfo/pypy-commit
