-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
Speed up isinstance on union types #91603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Wow I'm surprised it's that much slower. Out of curiosity, in the C code, why not pass the tuple |
def isinstance(inst, cls):
if type(inst) is cls:
return True
if type(cls) is type:
return type(inst) in cls.mro()
if tuple in type(cls).mro():
return any(isinstance(inst, c) for c in cls)
if hasattr(type(cls), "__instancecheck__"):
return type(cls).__instancecheck__(cls, inst)
if type in type(cls).mro():
...
raise TypeError
|
@JelleZijlstra thanks for the explanation. I was wondering if we can do something like this in C: def isinstance(inst, cls):
if type(inst) is cls:
return True
if type(cls) is type:
return type(inst) in cls.mro()
if type(cls) is tuple:
for t in cls:
isinstance(cls, t)
...
raise TypeError After: def isinstance(inst, cls):
if type(inst) is cls:
return True
if type(cls) is type:
return type(inst) in cls.mro()
if type(cls) is types.UnionType:
cls = cls.__args__
# fall through and let the code below handle this
if type(cls) is tuple:
for t in cls:
isinstance(cls, t)
...
raise TypeError |
@Fidget-Spinner @JelleZijlstra Currently, there is a difference in behavior between instance(1, (int, list[str]))
instance(1, int | list[str]) # will raise TypeError Should implementation should take into account such differences? |
@uriyyo that's pretty surprising, because |
I noticed a big chunk of the difference is actually from the construction of the
Reading the code, this is what
@Fidget-Spinner do you have ideas on how we can speed this up? A couple of ideas:
Regardless, it's still helpful to speed up the |
@JelleZijlstra I have an idea have we can speed up |
Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>
Reduce the complexity from O((M+N)^2) to O(M*N), where M and N are the length of __args__ for both operands (1 for operand which is not a UnionType). As a consequence, the complexity of parameter substitution in UnionType has been reduced from O(N^3) to O(N^2). Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com> Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@JelleZijlstra @Fidget-Spinner We introduced What do you think about using this function instead of accessing by pointer? As for me, it will be more readable and maintainable. For instance: PyObject *a_set = PySet_New(((unionobject*)a)->args); // old
PyObject *a_set = PySet_New(_Py_union_args(a)); // new I am happy to make PR with such improvement. |
There is no |
I just merged a PR using it. But yeah I agree with Serhiy that we don't need it if it's used internally. I think the benefit exists if it's used in an external file with no access to the |
PEP 604 allows
isinstance
on union types but it much slower compared to a tuple of types. This can be speed up with a fast path. Currently it is almost 4x slower, so there is room for improvement.Benchmarks:
The text was updated successfully, but these errors were encountered: