Improve InterfaceClass __hash__ performance

[jensens]

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).
Even if a single hash is very fast, this sums up to a real significant part of processing time.
Because the hash value calculated from name and __module__ never changes (afaik), it can be cached.

I took Py-Spy to analyze the performance.
On reindexing of an index in ZCatalog the __hash__ took ~9 seconds after 10000 samples before changes.
With a cached hash like in this implementation __hash__ takes ~4.7 seconds.
The whole reindex index process came down from 402s to 320s (1.26x faster).
The test performance python setup.py test goes from 0.614s down to 0.575s (1.07x faster).

[fschulze]

Looks good to me. If someone does anything which breaks this, they can be expected to adapt to the change.

[ale-rt]

I am testing this.
I will let you know but looks sweet!

[jaroel]

I'm pondering if it could be possible that an Interface instance is persisted, and then moved to a different module. How would that affect the hash?
In other words, would it be better/safer to use self._v_cached_hash?

[ctheune]

I think this is fine. I guess the fall-through path has become a tiny bit more expensive but the regular cache access obviously is much faster than before. Someone questioned whether name or module can change ... it's Python so people could manipulate this but I don't think it practically will.

[ale-rt]

I added this checkout on Plone5.2 found what could be a problem in some (not all) browser doctests (which are a functional test):

...
  File "/var/lib/jenkins/.buildout/eggs/plone.autoform-1.8.1-py3.7.egg/plone/autoform/utils.py", line 287, in processFields
    _process_fieldsets(form, schema, groups, all_fields, prefix, defaultGroup)
  File "/var/lib/jenkins/.buildout/eggs/plone.autoform-1.8.1-py3.7.egg/plone/autoform/utils.py", line 170, in _process_fieldsets
    group.fields += new_fields
  File "/var/lib/jenkins/.buildout/eggs/z3c.form-3.7.0-py3.7.egg/z3c/form/util.py", line 298, in __add__
    return self.__class__(self, other)
  File "/var/lib/jenkins/.buildout/eggs/z3c.form-3.7.0-py3.7.egg/z3c/form/field.py", line 136, in __init__
    raise ValueError("Duplicate name", name)
ValueError: ('Duplicate name', 'IDublinCore.creators')

Robot tests (which are also functional) do not suffer from this.

Just mentioning as a note, it might be that plone.autoform needs an update and Plone 5.2 is not really thought to work with zope.interface master.

Abyway for me it is good to go :)

[mgedmin]

lgtm

[jamadden]

I'm pondering if it could be possible that an Interface instance is persisted, and then moved to a different module. How would that affect the hash?
In other words, would it be better/safer to use self._v_cached_hash?

InterfaceClass defines __reduce__ which is used in preference to the default pickle behaviour of saving an instance's __dict__. __reduce__ simply returns the name of the interface (which is automatically combined with its __module__ by the pickle machinery) so unpickling turns into a global lookup of the object with the same name and module. Also, _v_ attributes are only special to subclasses of persistent.Persistent, which InterfaceClass isn't.

So this would matter only to a a subclass of InterfaceClass that overrides __reduce__, potentially by subclassing Persistent.

On current master, it's not possible to subclass both Persistent and InterfaceClass:

>>> from zope import interface
>>> from persistent import Persistent
>>> class PersistentInterfaceClass(Persistent, 
          interface.interface.InterfaceClass):
…        pass 
TypeError: multiple bases have instance lay-out conflict

On current released versions, it's not possible to pickle such an object because there are things in __dict__ that can't be pickled:

>>> from zope import interface
>>> from persistent import Persistent
>>> class PersistentInterfaceClass(Persistent,
           interface.interface.InterfaceClass):
...     pass
...
>>> class IP(metaclass=PersistentInterfaceClass): pass
...
>>> IP
<__main__.PersistentInterfaceClass object at 0x10f5c59d0>
>>> type(IP)
<class '__main__.PersistentInterfaceClass'>
>>> import pickle
>>> pickle.dumps(IP)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: Can't pickle local object 'WeakKeyDictionary.__init__.<locals>.remove'

If you switch the order of inheritance around you can pickle it, but it goes back to pickling by name using __reduce__ and ignoring the persistence machinery. ZODB fails to pickle IP and also fails to pickle the reversed order of inheritance because there's no __getstate__ defined.

So it would take subclassing Persistent and InterfaceClass in the right order, and also defining __getstate__ in this subclass to attempt to pickle an interface in this way. Is anyone aware of something that does that, or that has a use case to do that?

[jamadden]

LGTM.