classification
Title: Move quick test in PyObject_IsSubClass outside of PyType_CheckExact guard
Type: performance Stage:
Components: Interpreter Core Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Jim Fasarakis-Hilliard, georg.brandl, pitrou, rhettinger, serhiy.storchaka
Priority: low Keywords:

Created on 2017-05-02 14:30 by Jim Fasarakis-Hilliard, last changed 2018-12-11 16:13 by serhiy.storchaka.

Messages (8)
msg292764 - (view) Author: Jim Fasarakis-Hilliard (Jim Fasarakis-Hilliard) * Date: 2017-05-02 14:30
Currently the first lines in PyObject_IsSubClass are:

    /* We know what type's __subclasscheck__ does. */
    if (PyType_CheckExact(cls)) {
        /* Quick test for an exact match */
        if (derived == cls)
            return 1;
        return recursive_issubclass(derived, cls);
    }

The if (derived == cls) runs only if PyType_CheckExact is True which doesn't hold for any classes with a custom metaclass. As a result, a check of the form issubclass(Sequence, Sequence) will take a slow path that invokes __subclasscheck__.

I'm not sure if this was placed inside there due to some wild edge-case, though. PyObject_IsInstance uses the same trick and does so outside the if (PyType_CheckExact(cls)) check.

I'll gladly submit a PR if this indeed needs fixing and not by design.
msg293244 - (view) Author: Jim Fasarakis-Hilliard (Jim Fasarakis-Hilliard) * Date: 2017-05-08 17:24
Additional question, are tests required to check this behavior? (Also, bumping)
msg293379 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-05-10 05:38
Will deeply grokking this code, I would say to leave it alone.  The test seems reasonable and is very fast (likely free of cost on many machines).
msg293390 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-05-10 08:31
Jim doesn't propose to remove fast test, it proposes to add a test for fast path for avoiding invoking __subclasscheck__ (this is slow). This can change semantic, but isinstance() already contains such fast path.

This is similar to testing "elem is seq[i]" before "elem == seq[i]" when search in sequences. This optimization changes semantic for NaNs. Does a "Not-A-Class" class which is not a subclass of itself (issubclass(C, C) returns False) makes any sense?
msg293422 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-05-10 14:40
My point was that it isn't wise to propose PRs for code you don't fully understand (he said, "I'm not sure if this was placed inside there due to some wild edge-case,").
msg293471 - (view) Author: Jim Fasarakis-Hilliard (Jim Fasarakis-Hilliard) * Date: 2017-05-11 02:50
> he said, "I'm not sure if this was placed inside there due to some wild edge-case,"

As a new contributor, I'll always lean on the side of me not seeing something rather than confidently stating that something is definitely wrong. I'm new, the code-base is 20 years old and worked on by a dozen of experienced devs :-)

>  Does a "Not-A-Class" class which is not a subclass of itself (issubclass(C, C) returns False) makes any sense?

I'm not sure. Apart from the performance impact, there's a behavioral discrepancy between isinstance and issubclass as you also stated. 

In isinstance, __instancecheck__ doesn't *always* get a chance to override the behavior. The `if type(inst) == Cls` [1] stops it before it gets a chance. 
In issubclass, __subclasscheck__ does override it:

    class Meta(type):
        def __instancecheck__(self, other):
            print("invoked")
            return False
        def __subclasscheck__(self, other):
            print("invoked")
            return False

    class Cls(metaclass=Meta):
        pass

    isinstance(Cls(), Cls)
    True

    issubclass(Cls, Cls)
    invoked
    False

So, I guess the question might be re-framed to: Is it guaranteed that __instancecheck__ and __subclasscheck__ are *always* called?

If yes: PyObject_IsInstance should be tweaked.
If no:  PyObject_IsSubclass should be tweaked.

p.s Should I maybe move this to python-dev?

[1]: https://github.com/python/cpython/blob/master/Objects/abstract.c#L2338
msg306785 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-11-23 07:06
Yes, please move this to Python-Dev.
msg331638 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-12-11 16:13
Any progress?
History
Date User Action Args
2018-12-11 16:13:42serhiy.storchakasetmessages: + msg331638
versions: + Python 3.8, - Python 3.7
2017-11-23 07:06:21serhiy.storchakasetmessages: + msg306785
2017-05-11 02:50:16Jim Fasarakis-Hilliardsetmessages: + msg293471
2017-05-10 14:40:31rhettingersetmessages: + msg293422
2017-05-10 08:31:18serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg293390
2017-05-10 05:38:33rhettingersetpriority: normal -> low
nosy: + georg.brandl, pitrou
messages: + msg293379

2017-05-09 16:32:30rhettingersetnosy: + rhettinger
2017-05-08 17:24:09Jim Fasarakis-Hilliardsetmessages: + msg293244
2017-05-02 14:35:12serhiy.storchakasettype: behavior -> performance
title: Move quick test in PyObject_IsSubClass outside if PyType_CheckExact guard -> Move quick test in PyObject_IsSubClass outside of PyType_CheckExact guard
2017-05-02 14:30:45Jim Fasarakis-Hilliardcreate