From ab0466e56c6328407c3839a3a392cb127dbdb282 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Thu, 23 Jan 2020 10:24:12 -0600 Subject: [PATCH 1/4] Move Declaration, Specification and ClassProvides to __slots__. In a test of 6000 modules that load 2245 InterfaceClass objects and produce 2233 ClassProvides instances, this saves about 1% total memory usage in Python 2.7. --- .../_zope_interface_coptimizations.c | 34 ++++++++++- src/zope/interface/declarations.py | 22 +++---- src/zope/interface/interface.py | 58 ++++++++++++++----- src/zope/interface/tests/test_declarations.py | 14 ++--- src/zope/interface/tests/test_interface.py | 2 +- 5 files changed, 93 insertions(+), 37 deletions(-) diff --git a/src/zope/interface/_zope_interface_coptimizations.c b/src/zope/interface/_zope_interface_coptimizations.c index db0b4431..e308cbd1 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 991fb95c..cc20f5a2 100644 --- a/src/zope/interface/declarations.py +++ b/src/zope/interface/declarations.py @@ -62,16 +62,11 @@ def __call__(self, ob): 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 7a852f83..d3074c5e 100644 --- a/src/zope/interface/interface.py +++ b/src/zope/interface/interface.py @@ -96,9 +96,27 @@ def setTaggedValue(self, 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 +146,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,14 +196,20 @@ 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=()): - self._implied = {} self.dependents = weakref.WeakKeyDictionary() + self._bases = () + self._implied = {} + self._v_attrs = None + self.__iro__ = () + self.__sro__ = () + self.__bases__ = tuple(bases) def subscribe(self, dependent): @@ -201,25 +230,21 @@ def __setBases(self, bases): 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() @@ -245,6 +270,10 @@ def changed(self, originally_changed): for dependent in tuple(self.dependents.keys()): 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. @@ -274,9 +303,8 @@ def weakref(self, callback=None): 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 +314,8 @@ def get(self, name, default=None): 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 5b290f3d..887a5cb0 100644 --- a/src/zope/interface/tests/test_declarations.py +++ b/src/zope/interface/tests/test_declarations.py @@ -101,31 +101,31 @@ def test_ctor_w_implements_in_bases(self): 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 @@ def __call__(self): 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 @@ def test_no_existing_implements(self): 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 69b1d475..f5a57b42 100644 --- a/src/zope/interface/tests/test_interface.py +++ b/src/zope/interface/tests/test_interface.py @@ -370,7 +370,7 @@ class I(Interface): 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): From 7f6638cf5113f9f6ecb51116bbb72fdbc3f3ce06 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Thu, 23 Jan 2020 10:57:00 -0600 Subject: [PATCH 2/4] Specifications with no dependents are common (4700 out of 7000 in this example), so avoid allocating a WeakKeyDictionary until needed. This saves another 2% or so. --- .../_zope_interface_coptimizations.c | 8 ++--- src/zope/interface/interface.py | 34 ++++++++++++++----- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/src/zope/interface/_zope_interface_coptimizations.c b/src/zope/interface/_zope_interface_coptimizations.c index e308cbd1..4794e3f2 100644 --- a/src/zope/interface/_zope_interface_coptimizations.c +++ b/src/zope/interface/_zope_interface_coptimizations.c @@ -272,7 +272,7 @@ typedef struct { The remainder aren't used in C code but must be stored here to prevent instance layout conflicts. */ - PyObject* dependents; + PyObject* _dependents; PyObject* _bases; PyObject* _v_attrs; PyObject* __iro__; @@ -287,7 +287,7 @@ static int Spec_traverse(Spec* self, visitproc visit, void* arg) { Py_VISIT(self->_implied); - Py_VISIT(self->dependents); + Py_VISIT(self->_dependents); Py_VISIT(self->_v_attrs); Py_VISIT(self->__iro__); Py_VISIT(self->__sro__); @@ -298,7 +298,7 @@ static int Spec_clear(Spec* self) { Py_CLEAR(self->_implied); - Py_CLEAR(self->dependents); + Py_CLEAR(self->_dependents); Py_CLEAR(self->_v_attrs); Py_CLEAR(self->__iro__); Py_CLEAR(self->__sro__); @@ -408,7 +408,7 @@ static struct PyMethodDef Spec_methods[] = { static PyMemberDef Spec_members[] = { {"_implied", T_OBJECT_EX, offsetof(Spec, _implied), 0, ""}, - {"dependents", T_OBJECT_EX, offsetof(Spec, dependents), 0, ""}, + {"_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, ""}, diff --git a/src/zope/interface/interface.py b/src/zope/interface/interface.py index d3074c5e..8317a7f7 100644 --- a/src/zope/interface/interface.py +++ b/src/zope/interface/interface.py @@ -111,7 +111,7 @@ class SpecificationBase(object): # Things used here. '_implied', # Things used in Specification. - 'dependents', + '_dependents', '_bases', '_v_attrs', '__iro__', @@ -203,7 +203,15 @@ class Specification(SpecificationBase): providedBy = SpecificationBase.providedBy def __init__(self, bases=()): - self.dependents = weakref.WeakKeyDictionary() + # 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 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._v_attrs = None @@ -212,17 +220,26 @@ def __init__(self, bases=()): 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 @@ -267,7 +284,7 @@ def changed(self, originally_changed): 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 @@ -285,7 +302,6 @@ def interfaces(self): seen[interface] = 1 yield interface - def extends(self, interface, strict=True): """Does the specification extend the given interface? From 7a34a32a3b66073f91ffe978132ae71783a62630 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Thu, 23 Jan 2020 11:38:41 -0600 Subject: [PATCH 3/4] Avoid allocating space for tagged values unless they're used. This saves another few percent. --- CHANGES.rst | 25 +++++++++++++++++++------ src/zope/interface/interface.py | 12 +++++++++--- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 9205b883..77fecbdd 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 `_. +- 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 + `_. + - 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 `_. -- 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 `_. +- 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 `_. + 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/interface.py b/src/zope/interface/interface.py index 8317a7f7..106a60d0 100644 --- a/src/zope/interface/interface.py +++ b/src/zope/interface/interface.py @@ -67,7 +67,9 @@ def __init__(self, __name__, __doc__=''): 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,18 +81,22 @@ def getDoc(self): 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 From 2c359081237f5d8a87de223e32e380d83b4bd26f Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Mon, 27 Jan 2020 07:44:29 -0600 Subject: [PATCH 4/4] Whitespace. --- src/zope/interface/_zope_interface_coptimizations.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zope/interface/_zope_interface_coptimizations.c b/src/zope/interface/_zope_interface_coptimizations.c index 4794e3f2..f4f9cced 100644 --- a/src/zope/interface/_zope_interface_coptimizations.c +++ b/src/zope/interface/_zope_interface_coptimizations.c @@ -309,7 +309,7 @@ static void Spec_dealloc(Spec* self) { if (self->weakreflist != NULL) { - PyObject_ClearWeakRefs(OBJECT(self)); + PyObject_ClearWeakRefs(OBJECT(self)); } Spec_clear(self); Py_TYPE(self)->tp_free(OBJECT(self));