Merge pull request #155 from zopefoundation/slots

More memory optimizations for a 6-7% reduction.

6 files changed, 144 insertions, 53 deletions

diff --git a/CHANGES.rst b/CHANGES.rst
index 9205b88..77fecbd 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -11,19 +11,32 @@
   value forces the Python implementation to be used, ignoring the C
   extensions. See `PR 151 <https://github.com/zopefoundation/zope.interface/pull/151>`_.
 
+- Cache the result of ``__hash__`` method in ``InterfaceClass`` as a
+  speed optimization. The method is called very often (i.e several
+  hundred thousand times during Plone 5.2 startup). Because the hash value never
+  changes it can be cached. This improves test performance from 0.614s
+  down to 0.575s (1.07x faster). In a real world Plone case a reindex
+  index came down from 402s to 320s (1.26x faster). See `PR 156
+  <https://github.com/zopefoundation/zope.interface/pull/156>`_.
+
 - Change the C classes ``SpecificationBase`` and its subclass
   ``ClassProvidesBase`` to store implementation attributes in their structures
   instead of their instance dictionaries. This eliminates the use of
   an undocumented private C API function, and helps make some
   instances require less memory. See `PR 154 <https://github.com/zopefoundation/zope.interface/pull/154>`_.
 
-- Performance optimization of ``__hash__`` method on ``InterfaceClass``.
-  The method is called very often (i.e several 100.000 times on a Plone 5.2
-  startup). Because the hash value never changes it can be cached.
-  This improves test performance from 0.614s down to 0.575s (1.07x faster).
-  In a real world Plone case a reindex index came down from 402s to 320s (1.26x faster).
-  See `PR 156 <https://github.com/zopefoundation/zope.interface/pull/156>`_.
+- Reduce memory usage in other ways based on observations of usage
+  patterns in Zope (3) and Plone code bases.
+
+  - Specifications with no dependents are common (more than 50%) so
+    avoid allocating a ``WeakKeyDictionary`` unless we need it.
+  - Likewise, tagged values are relatively rare, so don't allocate a
+    dictionary to hold them until they are used.
+  - Use ``__slots___`` or the C equivalent ``tp_members`` in more
+    common places. See `PR 155 <https://github.com/zopefoundation/zope.interface/pull/155>`_.
 
+  The changes in this release resulted in a 7% memory reduction after
+  loading about 6,000 modules that define about 2,200 interfaces.
 
 4.7.1 (2019-11-11)
 ==================
diff --git a/src/zope/interface/_zope_interface_coptimizations.c b/src/zope/interface/_zope_interface_coptimizations.c
index db0b443..f4f9cce 100644
--- a/src/zope/interface/_zope_interface_coptimizations.c
+++ b/src/zope/interface/_zope_interface_coptimizations.c
@@ -259,6 +259,7 @@ providedBy(PyObject *ignored, PyObject *ob)
 
 typedef struct {
     PyObject_HEAD
+    PyObject* weakreflist;
     /*
       In the past, these fields were stored in the __dict__
       and were technically allowed to contain any Python object, though
@@ -267,6 +268,15 @@ typedef struct {
       make any assumptions about contents.
     */
     PyObject* _implied;
+    /*
+      The remainder aren't used in C code but must be stored here
+      to prevent instance layout conflicts.
+    */
+    PyObject* _dependents;
+    PyObject* _bases;
+    PyObject* _v_attrs;
+    PyObject* __iro__;
+    PyObject* __sro__;
 } Spec;
 
 /*
@@ -277,6 +287,10 @@ static int
 Spec_traverse(Spec* self, visitproc visit, void* arg)
 {
     Py_VISIT(self->_implied);
+    Py_VISIT(self->_dependents);
+    Py_VISIT(self->_v_attrs);
+    Py_VISIT(self->__iro__);
+    Py_VISIT(self->__sro__);
     return 0;
 }
 
@@ -284,12 +298,19 @@ static int
 Spec_clear(Spec* self)
 {
     Py_CLEAR(self->_implied);
+    Py_CLEAR(self->_dependents);
+    Py_CLEAR(self->_v_attrs);
+    Py_CLEAR(self->__iro__);
+    Py_CLEAR(self->__sro__);
     return 0;
 }
 
 static void
 Spec_dealloc(Spec* self)
 {
+    if (self->weakreflist != NULL) {
+        PyObject_ClearWeakRefs(OBJECT(self));
+    }
     Spec_clear(self);
     Py_TYPE(self)->tp_free(OBJECT(self));
 }
@@ -387,7 +408,12 @@ static struct PyMethodDef Spec_methods[] = {
 
 static PyMemberDef Spec_members[] = {
   {"_implied", T_OBJECT_EX, offsetof(Spec, _implied), 0, ""},
-  {NULL}
+  {"_dependents", T_OBJECT_EX, offsetof(Spec, _dependents), 0, ""},
+  {"_bases", T_OBJECT_EX, offsetof(Spec, _bases), 0, ""},
+  {"_v_attrs", T_OBJECT_EX, offsetof(Spec, _v_attrs), 0, ""},
+  {"__iro__", T_OBJECT_EX, offsetof(Spec, __iro__), 0, ""},
+  {"__sro__", T_OBJECT_EX, offsetof(Spec, __sro__), 0, ""},
+  {NULL},
 };
 
 
@@ -417,7 +443,7 @@ static PyTypeObject SpecType = {
         /* tp_traverse       */ (traverseproc)Spec_traverse,
         /* tp_clear          */ (inquiry)Spec_clear,
         /* tp_richcompare    */ (richcmpfunc)0,
-        /* tp_weaklistoffset */ (long)0,
+        /* tp_weaklistoffset */ offsetof(Spec, weakreflist),
         /* tp_iter           */ (getiterfunc)0,
         /* tp_iternext       */ (iternextfunc)0,
         /* tp_methods        */ Spec_methods,
@@ -1779,3 +1805,7 @@ PyInit__zope_interface_coptimizations(void)
   return init();
 }
 #endif
+
+#ifdef __clang__
+#pragma clang diagnostic pop
+#endif
diff --git a/src/zope/interface/declarations.py b/src/zope/interface/declarations.py
index 991fb95..cc20f5a 100644
--- a/src/zope/interface/declarations.py
+++ b/src/zope/interface/declarations.py
@@ -62,16 +62,11 @@ class named(object):
 class Declaration(Specification):
     """Interface declarations"""
 
+    __slots__ = ()
+
     def __init__(self, *interfaces):
         Specification.__init__(self, _normalizeargs(interfaces))
 
-    def changed(self, originally_changed):
-        Specification.changed(self, originally_changed)
-        try:
-            del self._v_attrs
-        except AttributeError:
-            pass
-
     def __contains__(self, interface):
         """Test whether an interface is in the specification
         """
@@ -625,9 +620,12 @@ def noLongerProvides(object, interface):
 
 
 @_use_c_impl
-class ClassProvidesBase(object):
-    # In C, this extends SpecificationBase, so its kind of weird here that it
-    # doesn't.
+class ClassProvidesBase(SpecificationBase):
+
+    __slots__ = (
+        '_cls',
+        '_implements',
+    )
 
     def __get__(self, inst, cls):
         if cls is self._cls:
@@ -916,6 +914,8 @@ def _normalizeargs(sequence, output=None):
 
     return output
 
-_empty = Declaration()
+# XXX: Declarations are mutable, allowing adjustments to their __bases__
+# so having one as a singleton may not be a great idea.
+_empty = Declaration() # type: Declaration
 
 objectSpecificationDescriptor = ObjectSpecificationDescriptor()
diff --git a/src/zope/interface/interface.py b/src/zope/interface/interface.py
index 7a852f8..106a60d 100644
--- a/src/zope/interface/interface.py
+++ b/src/zope/interface/interface.py
@@ -67,7 +67,9 @@ class Element(object):
 
         self.__name__ = __name__
         self.__doc__ = __doc__
-        self.__tagged_values = {}
+        # Tagged values are rare, especially on methods or attributes.
+        # Deferring the allocation can save substantial memory.
+        self.__tagged_values = None
 
     def getName(self):
         """ Returns the name of the object. """
@@ -79,26 +81,48 @@ class Element(object):
 
     def getTaggedValue(self, tag):
         """ Returns the value associated with 'tag'. """
+        if not self.__tagged_values:
+            raise KeyError(tag)
         return self.__tagged_values[tag]
 
     def queryTaggedValue(self, tag, default=None):
         """ Returns the value associated with 'tag'. """
-        return self.__tagged_values.get(tag, default)
+        return self.__tagged_values.get(tag, default) if self.__tagged_values else default
 
     def getTaggedValueTags(self):
         """ Returns a list of all tags. """
-        return self.__tagged_values.keys()
+        return self.__tagged_values.keys() if self.__tagged_values else ()
 
     def setTaggedValue(self, tag, value):
         """ Associates 'value' with 'key'. """
+        if self.__tagged_values is None:
+            self.__tagged_values = {}
         self.__tagged_values[tag] = value
 
 
 @_use_c_impl
 class SpecificationBase(object):
-
+    # This object is the base of the inheritance hierarchy for ClassProvides:
+    #
+    # ClassProvides < ClassProvidesBase, Declaration
+    # Declaration < Specification < SpecificationBase
+    # ClassProvidesBase < SpecificationBase
+    #
+    # In order to have compatible instance layouts, we need to declare
+    # the storage used by Specification and Declaration here (and
+    # those classes must have ``__slots__ = ()``); fortunately this is
+    # not a waste of space because those are the only two inheritance
+    # trees. These all translate into tp_members in C.
     __slots__ = (
+        # Things used here.
         '_implied',
+        # Things used in Specification.
+        '_dependents',
+        '_bases',
+        '_v_attrs',
+        '__iro__',
+        '__sro__',
+        '__weakref__',
     )
 
     def providedBy(self, ob):
@@ -128,6 +152,11 @@ class InterfaceBase(object):
     """Base class that wants to be replaced with a C base :)
     """
 
+    __slots__ = ()
+
+    def _call_conform(self, conform):
+        raise NotImplementedError
+
     def __call__(self, obj, alternate=_marker):
         """Adapt an object to the interface
         """
@@ -173,27 +202,50 @@ class Specification(SpecificationBase):
     Specifications are mutable.  If you reassign their bases, their
     relations with other specifications are adjusted accordingly.
     """
+    __slots__ = ()
 
     # Copy some base class methods for speed
     isOrExtends = SpecificationBase.isOrExtends
     providedBy = SpecificationBase.providedBy
 
     def __init__(self, bases=()):
+        # There are many leaf interfaces with no dependents,
+        # and a few with very many. It's a heavily left-skewed
+        # distribution. In a survey of Plone and Zope related packages
+        # that loaded 2245 InterfaceClass objects and 2235 ClassProvides
+        # instances, there were a total of 7000 Specification objects created.
+        # 4700 had 0 dependents, 1400 had 1, 382 had 2 and so on. Only one
+        # for <type> had 1664. So there's savings to be had deferring
+        # the creation of dependents.
+        self._dependents = None # type: weakref.WeakKeyDictionary
+        self._bases = ()
         self._implied = {}
-        self.dependents = weakref.WeakKeyDictionary()
+        self._v_attrs = None
+        self.__iro__ = ()
+        self.__sro__ = ()
+
         self.__bases__ = tuple(bases)
 
+    @property
+    def dependents(self):
+        if self._dependents is None:
+            self._dependents = weakref.WeakKeyDictionary()
+        return self._dependents
+
     def subscribe(self, dependent):
-        self.dependents[dependent] = self.dependents.get(dependent, 0) + 1
+        self._dependents[dependent] = self.dependents.get(dependent, 0) + 1
 
     def unsubscribe(self, dependent):
-        n = self.dependents.get(dependent, 0) - 1
+        try:
+            n = self._dependents[dependent]
+        except TypeError:
+            raise KeyError(dependent)
+        n -= 1
         if not n:
             del self.dependents[dependent]
-        elif n > 0:
-            self.dependents[dependent] = n
         else:
-            raise KeyError(dependent)
+            assert n > 0
+            self.dependents[dependent] = n
 
     def __setBases(self, bases):
         # Remove ourselves as a dependent of our old bases
@@ -201,25 +253,21 @@ class Specification(SpecificationBase):
             b.unsubscribe(self)
 
         # Register ourselves as a dependent of our bases
-        self.__dict__['__bases__'] = bases
+        self._bases = bases
         for b in bases:
             b.subscribe(self)
 
         self.changed(self)
 
     __bases__ = property(
-
-        lambda self: self.__dict__.get('__bases__', ()),
+        lambda self: self._bases,
         __setBases,
         )
 
     def changed(self, originally_changed):
         """We, or something we depend on, have changed
         """
-        try:
-            del self._v_attrs
-        except AttributeError:
-            pass
+        self._v_attrs = None
 
         implied = self._implied
         implied.clear()
@@ -242,9 +290,13 @@ class Specification(SpecificationBase):
             implied[ancestor] = ()
 
         # Now, advise our dependents of change:
-        for dependent in tuple(self.dependents.keys()):
+        for dependent in tuple(self._dependents.keys() if self._dependents else ()):
             dependent.changed(originally_changed)
 
+        # Just in case something called get() at some point
+        # during that process and we have a cycle of some sort
+        # make sure we didn't cache incomplete results.
+        self._v_attrs = None
 
     def interfaces(self):
         """Return an iterator for the interfaces in the specification.
@@ -256,7 +308,6 @@ class Specification(SpecificationBase):
                     seen[interface] = 1
                     yield interface
 
-
     def extends(self, interface, strict=True):
         """Does the specification extend the given interface?
 
@@ -274,9 +325,8 @@ class Specification(SpecificationBase):
     def get(self, name, default=None):
         """Query for an attribute description
         """
-        try:
-            attrs = self._v_attrs
-        except AttributeError:
+        attrs = self._v_attrs
+        if attrs is None:
             attrs = self._v_attrs = {}
         attr = attrs.get(name)
         if attr is None:
@@ -286,10 +336,8 @@ class Specification(SpecificationBase):
                     attrs[name] = attr
                     break
 
-        if attr is None:
-            return default
-        else:
-            return attr
+        return default if attr is None else attr
+
 
 class InterfaceClass(Element, InterfaceBase, Specification):
     """Prototype (scarecrow) Interfaces Implementation."""
diff --git a/src/zope/interface/tests/test_declarations.py b/src/zope/interface/tests/test_declarations.py
index 5b290f3..887a5cb 100644
--- a/src/zope/interface/tests/test_declarations.py
+++ b/src/zope/interface/tests/test_declarations.py
@@ -101,31 +101,31 @@ class DeclarationTests(unittest.TestCase):
     def test_changed_wo_existing__v_attrs(self):
         decl = self._makeOne()
         decl.changed(decl) # doesn't raise
-        self.assertFalse('_v_attrs' in decl.__dict__)
+        self.assertIsNone(decl._v_attrs)
 
     def test_changed_w_existing__v_attrs(self):
         decl = self._makeOne()
         decl._v_attrs = object()
         decl.changed(decl)
-        self.assertFalse('_v_attrs' in decl.__dict__)
+        self.assertIsNone(decl._v_attrs)
 
     def test___contains__w_self(self):
         from zope.interface.interface import InterfaceClass
         IFoo = InterfaceClass('IFoo')
         decl = self._makeOne()
-        self.assertFalse(decl in decl)
+        self.assertNotIn(decl, decl)
 
     def test___contains__w_unrelated_iface(self):
         from zope.interface.interface import InterfaceClass
         IFoo = InterfaceClass('IFoo')
         decl = self._makeOne()
-        self.assertFalse(IFoo in decl)
+        self.assertNotIn(IFoo, decl)
 
     def test___contains__w_base_interface(self):
         from zope.interface.interface import InterfaceClass
         IFoo = InterfaceClass('IFoo')
         decl = self._makeOne(IFoo)
-        self.assertTrue(IFoo in decl)
+        self.assertIn(IFoo, decl)
 
     def test___iter___empty(self):
         decl = self._makeOne()
@@ -454,7 +454,7 @@ class Test_implementedByFallback(unittest.TestCase):
         self.assertTrue(spec.inherit is foo)
         self.assertTrue(foo.__implemented__ is spec)
         self.assertTrue(foo.__providedBy__ is objectSpecificationDescriptor)
-        self.assertFalse('__provides__' in foo.__dict__)
+        self.assertNotIn('__provides__', foo.__dict__)
 
     def test_w_None_no_bases_w_class(self):
         from zope.interface.declarations import ClassProvides
@@ -601,7 +601,7 @@ class Test__implements_advice(unittest.TestCase):
         class Foo(object):
             __implements_advice_data__ = ((IFoo,), classImplements)
         self._callFUT(Foo)
-        self.assertFalse('__implements_advice_data__' in Foo.__dict__)
+        self.assertNotIn('__implements_advice_data__', Foo.__dict__)
         self.assertIsInstance(Foo.__implemented__,  Implements)
         self.assertEqual(list(Foo.__implemented__), [IFoo])
 
diff --git a/src/zope/interface/tests/test_interface.py b/src/zope/interface/tests/test_interface.py
index 69b1d47..f5a57b4 100644
--- a/src/zope/interface/tests/test_interface.py
+++ b/src/zope/interface/tests/test_interface.py
@@ -370,7 +370,7 @@ class SpecificationTests(unittest.TestCase):
         spec._v_attrs = 'Foo'
         spec._implied[I] = ()
         spec.changed(spec)
-        self.assertTrue(getattr(spec, '_v_attrs', self) is self)
+        self.assertIsNone(spec._v_attrs)
         self.assertFalse(I in spec._implied)
 
     def test_interfaces_skips_already_seen(self):