Move Interface hashing and comparison to C; 2.5 to 15x speedup in micro benchmarks

[jamadden]

Included benchmark numbers:

Current master, Python 3.8:

.....................
contains (empty dict): Mean +- std dev: 198 ns +- 5 ns
.....................
contains (populated dict): Mean +- std dev: 197 ns +- 6 ns
.....................
contains (populated list): Mean +- std dev: 53.1 us +- 1.2 us

This code:

.....................
contains (empty dict): Mean +- std dev: 77.9 ns +- 2.3 ns
.....................
contains (populated dict): Mean +- std dev: 78.4 ns +- 3.1 ns
.....................
contains (populated list): Mean +- std dev: 3.69 us +- 0.08 us

So anywhere from 2.5 to 15x faster. Not sure how that will translate to larger (macro) benchmarks, but I'm hopeful. I also need to do some sorting benchmarks (e.g., using interfaces as keys in BTrees).

To make this work, InterfaceBase had to extend SpecificationBase to get a consistent class layout, and that's where the attributes for __name__ and __module__ moved. Neither of those are a public classes, I think.

It turns out that messing with __module__ is nasty, tricky business, especially when you do it from C. Every time you define a new subclass, the descriptors that you set get overridden by the type machinery (PyType_Ready), and there are differences between Python 2 and 3. I'm using a data descriptor and a meta class right now to avoid that but I'm not super happy with that and would like to find a better way. (At least, maybe the data part of the descriptor isn't necessary?) It may be needed to move more code into C, I don't want a slowdown accessing __module__ either; copying around the standard PyGetSet or PyMember descriptors isn't enough because they don't work on the class object (so classImplements(InterfaceClass, IInterface) fails).

There's currently one example doctest failure due to a changed class name.

[jamadden]

I think this is ready for review. I experimented with several different approaches (visible in the commit history), and ultimately a metaclass was best from both a simplicity and a performance standpoint.

Speaking of performance: Equality and hashing are back on a par with identity-based versions. Improvements overall look pretty nice.

Benchmark	38-master-full	38-faster-meta
read __module__	41.8 ns	40.9 ns: 1.02x faster (-2%)
read __name__	41.8 ns	39.9 ns: 1.05x faster (-5%)
read providedBy	56.9 ns	58.4 ns: 1.03x slower (+3%)
query adapter (no registrations)	3.85 ms	2.95 ms: 1.31x faster (-24%)
query adapter (all trivial registrations)	4.62 ms	3.63 ms: 1.27x faster (-21%)
query adapter (registrations, wide inheritance)	51.8 us	42.2 us: 1.23x faster (-19%)
query adapter (registrations, deep inheritance)	52.0 us	41.7 us: 1.25x faster (-20%)
sort interfaces	234 us	29.9 us: 7.84x faster (-87%)
sort mixed	569 us	340 us: 1.67x faster (-40%)
contains (empty dict)	135 ns	55.2 ns: 2.44x faster (-59%)
contains (populated dict: interfaces)	137 ns	56.1 ns: 2.45x faster (-59%)
contains (populated list: interfaces)	39.7 us	2.96 us: 13.42x faster (-93%)
contains (populated dict: implementedBy)	137 ns	55.2 ns: 2.48x faster (-60%)
contains (populated list: implementedBy)	40.6 us	24.1 us: 1.68x faster (-41%)

Not significant (2): read __doc__; sort implementedBy

Sorting implementedBy objects didn't change; that makes sense, that part is still in Python.

I do see that I need to add some tests for the NotImplemented branch of comparisons.

[jamadden]

When running Plone's buildout.coredev test suite, I've had run times in the range of 22:40 to 26:11, across at least 5 different runs. With this branch, the run time was 19:06. (This is not a benchmark, and not repeatable, it's just an anecdote. But an encouraging one.)

There is one failure:

File "//plone.autoform-1.8.2-py2.7.egg/plone/autoform/tests/../autoform.rst", line 428, in autoform.rst
Failed example:
    IAnotherAnonymousSchema.__module__ = 'different.module'
Exception raised:
    Traceback (most recent call last):
      File "//lib/python2.7/doctest.py", line 1315, in __run
        compileflags, 1) in test.globs
      File "<doctest autoform.rst[75]>", line 1, in <module>
        IAnotherAnonymousSchema.__module__ = 'different.module'
    TypeError: readonly attribute

That test is explicitly checking for an older bug:

It is possible to have interfaces/schema that have an empty __name__
attribute, specifically in some cases where a schema is dynamically
created.  In such cases, it is possible to have a subclass of
AutoExtensibleForm implement a getPrefix() function as a sufficient
condition for group naming when autoGroups is True.

    Define some unnamed schema:

    >>> class IUnknownName(Interface):
    ...     this = schema.TextLine()
    ...
    >>> IUnknownName.__name__ = ''  # dynamic schema, empty __name__

    >>> class IAnotherAnonymousSchema(Interface):
    ...     that = schema.TextLine()
    ...
    >>> IAnotherAnonymousSchema.__name__ = ''

Fix for https://github.com/zopefoundation/zope.interface/issues/31

    >>> IAnotherAnonymousSchema.__module__ = 'different.module'

Because equality and hashing are both based on __name__ and __module__, and because various things derived from those two attributes are cached (the repr and hash code), reassigning them has long been a dangerous footgun. Since the test is talking about dynamic schemas, I think it could be changed to actually use dynamic schemas and not run into this problem:

IAnotherAnonymousSchema = InterfaceClass(
    '', 
    (Interface,) 
    {'that': schema.TextLine(), '__module__': 'different.module'})

[jensens]

Finally I found time to take a deeper look into this changes. Wow! This looks like it was quiet an effort to get to this well founded result. And I love it.

The problem in plone.autoform was fixed as suggested. I am even not sure if this is a good/valid use-case, whats is tested there, but the case is still supported.

I took a look at the code and can not find any problems so far. I would go for merge from my POV.
Given the complexity of this PR another reviewer would be probably a good idea.

[jamadden]

Thanks @jensens!

I'm happy to wait for more reviews. I agree it's a big PR, plus it touches C code, so more eyes are definitely better. I'd prefer not to wait more than a couple days, though. I'd really like to get these last few PRs merged and make a release before the end of the week.

[jensens]

I'd prefer not to wait more than a couple days, though. I'd really like to get these last few PRs merged and make a release before the end of the week.

+1

[mgedmin]

Well, this is a PR. Very interested reading. It probably works fine.

[mgedmin]

Is this a bugfix for code in master? Or something that's now necessary because of other changes in this PR?

[mgedmin]

k, now I think this is option B: changes in this PR (specifically, specifying a custom tp_alloc instead of 0)?

[jamadden]

When I added C fields to InterfaceBase, it was necessary to untrack it. As it now extends SpecificationBase I was going up the method hierarchy and realized it was necessary also to untrack SpecificationBase, since it also has fields that can contain arbitrary objects. So it's a bit of both? But really a bug in #155

[mgedmin]

This is now calling PyObject_GC_UnTrack(self) twice, once here directly and once through Spec_dealloc()?

I've no idea what PyObject_GC_UnTrack() does, so just wanted to bring this to your attention in case the double call is unintentional.

[jamadden]

Thanks. I double-checked, calling that API twice is fine.

[mgedmin]

I like the 'should', especially when talking about correctness.

[jamadden]

Hey, I just moved this comment from place to place. This PR didn't add it. It was added way back in #44 by some familiar names 😄

[mgedmin]

yeah, this should be except Exception:

[jamadden]

That'll take some changes in the C code too. There are several cases like that; some I fixed already. And when I say fixed, I mean, "made the Python code do except: like C 😢 )

[jensens]

If I get it right: Technically there is a difference in C between except Exception and a bare except?
But is there a difference semantically?

[mgedmin]

Compatibility with C code is a good argument for not touching this right now. (But it should be mentioned in the comment, next to the XXX -- something like "if you fix it here, please also do that in the_right_function() in filename.c".)

[jamadden]

Technically there is a difference in C between except Exception and a bare except?

Yes, much as in Python (where "bare except:" is a shortcut for except BaseException:). In C, a "bare except" is often not even explicitly written. If some function returns NULL (the universal indicator of failure), and that NULL result is ignored and the error is thrown away with PyErr_Clear(), that's basically like writing except: pass in Python. That looks like this:

  conform = PyObject_GetAttr(obj, str__conform__);
  if (conform != NULL) {
      return PyObject_CallMethodObjArgs(self, str_call_conform,
                                           conform, NULL);
  }
  else { // except:
    PyErr_Clear(); // pass
  }
  adapter = __adapt__(self, obj);

Catching specific exceptions is a bit more difficult and looks something like:

 othername = PyObject_GetAttrString(other, "__name__");
 if (!othername) {
     if (PyErr_Occurred() 
          && PyErr_ExceptionMatches(PyExc_AttributeError)) {
         PyErr_Clear();
     }
 }

Which is the rough equivalent of

try:
    othername = other.__name__
except AttributeError:
    pass

[mgedmin]

My head hurts.

[jamadden]

Hopefully not too bad?

[mgedmin]

So if this exception is caught and silently ignored...

[mgedmin]

... then we fail with an UnboundLocalError here?

Why not let the original exception go through?

[jensens]

Question is, when and why does sys._getframe(1).f_globals['__name__'] fail?

[jamadden]

Great points. I just cargo-culted that code from the __init__ of InterfaceClass. There, it's kinda prepared to deal with those errors (in that it won't fail immediately, but later on things like the repr and sorting are going to be messed up). Here, you're right, we're not prepared at all.