bpo-32544: Speed up hasattr() and getattr() #5173
Conversation
Include/object.h
Outdated
| @@ -536,6 +536,8 @@ PyAPI_FUNC(int) PyObject_SetAttr(PyObject *, PyObject *, PyObject *); | |||
| PyAPI_FUNC(int) PyObject_HasAttr(PyObject *, PyObject *); | |||
| #ifndef Py_LIMITED_API | |||
| PyAPI_FUNC(int) _PyObject_IsAbstract(PyObject *); | |||
| /* Same to PyObject_GetAttr(), but don't raise AttributeError. */ | |||
There was a problem hiding this comment.
I think this should read "Same as ..."
| @@ -567,7 +569,7 @@ PyAPI_FUNC(int) PyObject_CallFinalizerFromDealloc(PyObject *); | |||
| /* Same as PyObject_Generic{Get,Set}Attr, but passing the attributes | |||
| dict as the last parameter. */ | |||
| PyAPI_FUNC(PyObject *) | |||
| _PyObject_GenericGetAttrWithDict(PyObject *, PyObject *, PyObject *); | |||
| _PyObject_GenericGetAttrWithDict(PyObject *, PyObject *, PyObject *, int); | |||
There was a problem hiding this comment.
Would this change be backward incompatible? Or is this function is not covered by the backwards compatibility guarantees?
There was a problem hiding this comment.
AFAIK, APIs starting with underscore are private.
Third party libs shouldn't expect backward compatibility.
I don't know should I add Include/internal/object.h or not.
There was a problem hiding this comment.
OK, I see. But I don't know about Include/internal/object.h either. Probably @serhiy-storchaka can advise on this.
There was a problem hiding this comment.
Changing the signature of _PyObject_GenericGetAttrWithDict() LGTM. As for Include/internal/object.h, left this to other issue.
Objects/object.c
Outdated
| ret = (*tp->tp_getattro)(v, name); | ||
| } | ||
| } | ||
| else if (tp->tp_getattr != NULL) { |
There was a problem hiding this comment.
It looks like there is some code duplication with PyObject_GetAttr, does it make sense to factor it out?
There was a problem hiding this comment.
Hm, how do you think this?
https://gist.github.com/methane/4adf85086798c2c2a8ee59181474fb29
There was a problem hiding this comment.
@methane Yes this idea looks good.
|
Also I think we need a NEWS item for this. |
Objects/object.c
Outdated
| } | ||
| if (tp->tp_getattro != NULL) { | ||
| if (tp->tp_getattro == PyObject_GenericGetAttr) { | ||
| ret = _PyObject_GenericGetAttrWithDict(v, name, NULL, 1); |
There was a problem hiding this comment.
Can _PyObject_GenericGetAttrWithDict() raise an AttributeError? If can't, it is better to just return the result here. PyErr_ExceptionMatches(PyExc_AttributeError) has a non-zero cost, in some cases it can be significant. For example see bpo-31336.
There was a problem hiding this comment.
Yes, it may call descriptor. Any exception can be raised.
suppress=1 only suppress AttributeError raised from _PyObject_GenericGetAttrWithDict
directly.
There was a problem hiding this comment.
I changed _PyObject_GenericGetAttrWithDict() to suppress AttributeError
from descriptors too.
Since _PyObject_GenericGetAttrWithDict is performance critical function,
I'll run benchmark suite before merge this.
| @@ -0,0 +1,3 @@ | |||
| ``hasattr(obj, name)`` and ``getattr(obj, name, default)`` is about 4 times | |||
There was a problem hiding this comment.
I am not a native speaker, but I would use "are" here instead of "is".
Skip raising AttributeError when tp_getattro == PyObject_GenericGetAttr
https://bugs.python.org/issue32544