[pypy-commit] pypy remove-objspace-options: problems that the enabling mapdict by default found:

[cfbolz]

Author: Carl Friedrich Bolz <cfbolz at gmx.de>
Branch: remove-objspace-options
Changeset: r83835:9df69444009e
Date: 2016-04-23 13:37 +0300
http://bitbucket.org/pypy/pypy/changeset/9df69444009e/

Log:	problems that the enabling mapdict by default found:

	- the mapdict cache needed an extra lookup, that is fixed
	- looking up non-method things via the class is bad

diff --git a/pypy/objspace/std/callmethod.py b/pypy/objspace/std/callmethod.py
--- a/pypy/objspace/std/callmethod.py
+++ b/pypy/objspace/std/callmethod.py
@@ -23,6 +23,7 @@
 
 
 def LOOKUP_METHOD(f, nameindex, *ignored):
+    from pypy.objspace.std.typeobject import MutableCell
     #   stack before                 after
     #  --------------    --fast-method----fallback-case------------
     #
@@ -44,7 +45,18 @@
     w_type = space.type(w_obj)
     if w_type.has_object_getattribute():
         name = space.str_w(w_name)
-        w_descr = w_type.lookup(name)
+        # bit of a mess to use these internal functions, but it allows the
+        # mapdict caching below to work without an additional lookup
+        version_tag = w_type.version_tag()
+        if version_tag is None:
+            _, w_descr = w_type._lookup_where(name)
+            w_descr_cell = None
+        else:
+            _, w_descr_cell = w_type._pure_lookup_where_possibly_with_method_cache(
+                name, version_tag)
+            w_descr = w_descr_cell
+            if isinstance(w_descr, MutableCell):
+                w_descr = w_descr.unwrap_cell(space)
         if w_descr is None:
             # this handles directly the common case
             #   module.function(args..)
@@ -62,7 +74,8 @@
                     if not jit.we_are_jitted():
                         # let mapdict cache stuff
                         LOOKUP_METHOD_mapdict_fill_cache_method(
-                            space, f.getcode(), name, nameindex, w_obj, w_type)
+                            space, f.getcode(), name, nameindex, w_obj, w_type,
+                            w_descr_cell)
                     return
     if w_value is None:
         w_value = space.getattr(w_obj, w_name)
diff --git a/pypy/objspace/std/mapdict.py b/pypy/objspace/std/mapdict.py
--- a/pypy/objspace/std/mapdict.py
+++ b/pypy/objspace/std/mapdict.py
@@ -1011,22 +1011,15 @@
     return False
 
 def LOOKUP_METHOD_mapdict_fill_cache_method(space, pycode, name, nameindex,
-                                            w_obj, w_type):
+                                            w_obj, w_type, w_method):
+    if w_method is None or isinstance(w_method, MutableCell):
+        # don't cache the MutableCell XXX could be fixed
+        return
     version_tag = w_type.version_tag()
-    if version_tag is None:
-        return
+    assert version_tag is not None
     map = w_obj._get_mapdict_map()
     if map is None or isinstance(map.terminator, DevolvedDictTerminator):
         return
-    # We know here that w_obj.getdictvalue(space, name) just returned None,
-    # so the 'name' is not in the instance.  We repeat the lookup to find it
-    # in the class, this time taking care of the result: it can be either a
-    # quasi-constant class attribute, or actually a MutableCell --- which we
-    # must not cache.  (It should not be None here, but you never know...)
-    _, w_method = w_type._pure_lookup_where_possibly_with_method_cache(
-        name, version_tag)
-    if w_method is None or isinstance(w_method, MutableCell):
-        return
     _fill_cache(pycode, nameindex, map, version_tag, -1, w_method)
 
 # XXX fix me: if a function contains a loop with both LOAD_ATTR and
diff --git a/pypy/objspace/std/test/test_methodcache.py b/pypy/objspace/std/test/test_methodcache.py
--- a/pypy/objspace/std/test/test_methodcache.py
+++ b/pypy/objspace/std/test/test_methodcache.py
@@ -202,7 +202,8 @@
                 l = [type.__getattribute__(A, "__new__")(A)] * 10
                 __pypy__.reset_method_cache_counter()
                 for i, a in enumerate(l):
-                    assert a.f() == 42
+                    # use getattr to circumvent the mapdict cache
+                    assert getattr(a, "f")() == 42
                 cache_counter = __pypy__.method_cache_counter("f")
                 assert sum(cache_counter) == 10
                 if cache_counter == (9, 1):
@@ -225,9 +226,11 @@
                 assert a.x == i + 1
                 A.x += 1
             cache_counter = __pypy__.method_cache_counter("x")
-            assert cache_counter[0] >= 350
+            # XXX this is the bad case for the mapdict cache: looking up
+            # non-method attributes from the class
+            assert cache_counter[0] >= 450
             assert cache_counter[1] >= 1
-            assert sum(cache_counter) == 400
+            assert sum(cache_counter) == 500
 
             __pypy__.reset_method_cache_counter()
             a = A()